* feat: add Python code review rules
Add python.md rule doc for reviewing .py files and wire "**/*.py" into
the system rule map so Python files no longer fall back to default.md.
Rules follow the precision-first house style: security/correctness are
blocking, style is not, and noise-prone sections carry explicit
"do not report" guards.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix: align identity-comparison rule with precision-first principle
Split the identity/equality section so severity is explicit: `is`
against literals and `== True/False` are real correctness risks, while
`== None` vs `is None` is a style preference reported as minor. This
removes the section's conflict with the doc's own "style is non-blocking"
header.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat(action): extract reusable OpenCodeReview PR review GitHub Action
Consolidate the reusable-action work into one commit:
- Add composite action (action.yml at repo root for GitHub Marketplace;
helper at scripts/github-actions/post-review-comments.js) porting the
sticky summary, incremental posting, and retry idempotency logic.
- Add unit tests covering the ported idempotency behavior.
- Switch the in-repo CI workflow to use the reusable action.
- Add and refine example reusable workflows for consumers.
* ci(workflow): point ocr-review at root action.yml and quote boolean inputs
- Fix uses: to ./ now that action.yml lives at the repo root.
- Quote sticky_summary/incremental/upload_artifacts as strings to
match action.yml's input declarations (composite-action inputs are
always strings) and silence actionlint.
- Enable upload_artifacts for this workflow.
* docs(examples): point reusable demo at root action.yml
The example workflow referenced alibaba/open-code-review/action@v1,
but action.yml now lives at the repo root, so the /action subpath no
longer resolves. Use alibaba/open-code-review@v1 and update the stale
action/README.md comment to point at the root action.yml.
* docs(examples): sync README to root action.yml references
The example README still pointed at the relocated/deleted locations:
action.yml is now at the repo root, so update all 11
alibaba/open-code-review/action@v1 references to
alibaba/open-code-review@v1, and repoint the action/ directory and
action/README.md links to the root action.yml.
* fix(examples): prevent unrelated PR comments from canceling ocr-review
GitHub Actions evaluates concurrency before the job-level if-condition.
The flat group mapped every issue_comment event on a PR into the review's
group, so any comment (even a skipped conversation reply) canceled any
in-progress review.
Match the reusable demo's conditional group: PR events and human-authored
/open-code-review/@open-code-review comments share a per-PR group, while
non-matching comments fall back to a unique noop-<run_id> group that can
never collide with a real review.
* fix(action): address code-review findings across reusable PR review
- post-review-comments: parse retry delays via parseNonNegInt (0/negative fix);
paginate findExistingSummaryComment through readAllPages; remove dead
rangeOf and hasIssueCommentWithId (plus duplicated comment block)
- action.yml: move ${{ }} interpolations into env: (resolve refs, PR_NUM,
ocr_version); fail fast on PR head fetch instead of swallowing errors
- workflows: add timeout-minutes: 30; gate issue_comment on
author_association; tighten pr-context if to == 'issue_comment'
* fix(action): harden review posting after code review
- pass incremental_overlap_threshold via env to avoid github-script injection
- capture ocr review exit code directly instead of &&/|| chain
- drop redundant SUMMARY_MARKER prepend in postSummary (callers already add it)
- align example job if-condition bot check with its concurrency group
* fix(action): always upload review artifacts and capture ocr exit code
* fix(action): merge posting statistics into the summary header
The PR summary issue comment used to present two overlapping breakdowns:
a leading "posted as inline / posted as summary" header and a trailing
"📊 Posting Statistics" block. Their definitions overlapped (the header's
"summary" count included failures the trailer also listed as failed), and
when incremental filtering skipped comments the header counts no longer
summed to the total, making the summary hard to interpret.
Merge them into a single header whose four counts (inline / summary /
skipped / failed) are mutually exclusive and sum to the total, and drop
the trailing Posting Statistics section. buildSummaryBody now takes an
options object.
* fix(action): support local action resolution in container/self-hosted setups
- Checkout trusted base + mark workspace safe for pull_request_target so
the local `uses: ./` action can be resolved and loaded
- Check for git/Node.js and install git when missing, making the
composite action resilient across runner images
- Move Setup Node.js earlier and make it conditional on availability
- Resolve post-review-comments helper at runtime via
GITHUB_ACTION_PATH falling back to GITHUB_WORKSPACE, fixing helper
lookup for local actions where the action path is a host path
invisible inside containers
* refactor(examples): consolidate github_actions demo to reusable action
Drop the inline-script full-control demo; the renamed ocr-review.yml
(from ocr-review-reusable.yml) is now the single demo, invoking
alibaba/open-code-review@main.
Sync the README to the current implementation:
- normalize action refs to @main; point self-hosted-runner users to the
repo's own workflow (noting uses: ./ is internal-only)
- document config via action inputs (posting modes: sticky/incremental)
- update the comment-trigger if with defensive bot/author_association
guards and the concurrency mirror
- fix Example Output to cover the summary comment + inline comments
- replace the non-existent OCR_DEBUG debugging with
artifacts/outputs/ACTIONS_STEP_DEBUG
- use --replace-all for safe.directory
* fix(action): harden withRetry against silent undefined return
withRetry's for loop had no terminal return/throw after the loop body.
Although the current loop invariant (last attempt always throws, and
parseNonNegInt guards against negative MAX_RETRIES) makes fall-through
unreachable, an async function that falls through resolves to undefined,
which would surface as a confusing downstream TypeError for the read-API
callers that rely on it.
Capture lastErr in the loop and add an explicit terminal throw so any
future break of the invariant fails loudly instead of silently returning
undefined.
* docs(readme): document the reusable GitHub Action in CI/CD section
* fix(action): restore language config via a language input
The old inline workflow ran `ocr config set language English`, but the
composite action's Configure OCR step only set llm.extra_body, with no
language input. Add a language input (default English) and write it via
`ocr config set language` so review output language is no longer left
to the tool's default.
Addresses #337 (discussion_r3550069843).
* fix(action): warn when incremental comment listing hits page cap
listExistingReviewComments silently dropped comments beyond its 10-page
cap, unlike readAllPages which logs when truncation occurs. Add the
same max-page-limit warning after the loop so a partial walk during
incremental dedup is visible in the logs.
Addresses #337 (discussion_r3550069871).
* docs(readme): sync GitHub Action section to localized READMEs
* feat: adjust provider configuration panel component logic and dependencies.
* style: modified the display logic for step statuses in the configuration view.
Move MCP server to the current-state list now that it is supported, and
replace the MCP roadmap entry with a delegate mode that lets ocr run on
the host coding agent's subscription without a standalone LLM endpoint.
GitHub restricted the stargazers API (July 2026) to a repository's own
admins and collaborators, so the embedded star-history.com SVG can no
longer render for anonymous README viewers. Remove the Star History
section from README.md and all localized versions.
govulncheck flags GO-2026-5856 (Encrypted Client Hello privacy leak in
crypto/tls), present in the Go standard library through go1.26.4 and
fixed in go1.26.5. The CI and release workflows pin the golang:1.26.4
container image, so govulncheck fails with exit code 3 on every run.
Bump both workflow images to golang:1.26.5.
Go Report Card has been sunset by its maintainers; the badge endpoint now
returns "go report: retired" and the report page redirects to a farewell
notice, so the badge can never render a grade. Remove it from README.md and
all localized versions (zh-CN, ja-JP, ko-KR, ru-RU).
* feat(telemetry): add OTLP HTTP exporter and print TraceID
- Add HTTP/protobuf exporter support alongside existing gRPC exporter
- Route based on OTEL_EXPORTER_OTLP_PROTOCOL config (http/protobuf vs grpc)
- Print TraceID to stderr when telemetry is enabled for easier correlation
- Add corresponding unit tests
* feat(telemetry): add span coverage for LLM calls, tool execution, plan/filter phases
- Add StartLLMSpan / RecordLLMResult helpers (span.go), symmetric with
existing StartToolSpan / RecordToolResult
- Wrap LLM completion calls in llmloop.RunPerFile with llm.request spans
- Wrap all three tool execution paths in executeToolCall with
tool.execute.* spans (dynamic tools, code_comment sync/async, other tools)
- Add plan.execute span around executePlanPhase
- Add main.loop span around RunPerFile call in executeSubtask
- Add review_filter.execute span around executeReviewFilter, with
comments.before / comments.filtered attributes
- Record llm.error attribute on LLM failures for diagnosability
- Record review.repo / review.from / review.to / review.model on the
top-level review.run span
- Metrics (RecordLLMRequest / RecordToolCall) are preserved alongside
the new spans — they serve different purposes (aggregate dashboards
vs per-run diagnosis)
Verified end-to-end against Sunfire (OTLP HTTP gateway): full span tree
observed for review.run -> subtask.execute -> plan.execute/main.loop/
review_filter.execute -> llm.request/tool.execute.*
* fix(telemetry): address CR findings — span error handling, async span lifecycle, protocol robustness
- Add span.RecordError(err) to RecordLLMResult and RecordToolResult for
consistency with EndSpan
- Use OTel standard pattern (span.SetStatus + span.RecordError) in error
paths of review.run, plan.execute, main.loop, review_filter.execute
- Move async code_comment span end into pool.Submit callback so span
duration reflects actual execution time
- Unify time.Since(startTime) in code_comment error path to a single dur
- Remove http/json from supported OTLP protocols (not actually implemented)
- Add stderr warning when unknown OTLP protocol falls back to gRPC
* feat(telemetry): include trace_id in JSON output, restrict stderr to text format
- Add trace_id as top-level field in jsonOutput struct (omitempty)
- JSON format: trace_id in structured response for programmatic extraction
- Text format: TraceID printed to stderr for human debugging
- Telemetry disabled: trace_id field omitted entirely
* fix: address PR review findings
- loop.go: wrap async span lifecycle in defer to prevent leak on panic
- exporter.go: update parseOTLPEndpoint comment to reflect gRPC+HTTP usage
- scan_cmd.go: align traceID extraction and OTel error handling with review_cmd
- output.go/shared.go: propagate traceID to outputJSONNoFiles for consistency
- agent.go: move comments.filtered attribute before early return so 0 is
distinguishable from not-executed
* feat(telemetry): address PR review — http/json routing, LLM span coverage, trace_id tests
- Route http/json to HTTP exporter (Go OTel SDK HTTP transport only
supports protobuf serialization; users need HTTP transport, not JSON encoding)
- Add llm.request spans to executePlanPhase, executeReviewFilter, and
ReLocateComment with Usage nil-safety consistent with loop.go
- Add trace_id assertions to output helper tests and emitRunResult
end-to-end tests using real TracerProvider
* docs: add OTLP protocol selection and endpoint format to telemetry section
Sync across all 5 README language versions (en, zh-CN, ja-JP, ko-KR, ru-RU).
* fix: unify time.Since in async code_comment defer to single dur variable
Enhance the embedded viewer stylesheet per #304:
- Add automatic dark mode via `prefers-color-scheme` (no JS/toggle).
The full colour palette — surfaces, text, borders, accents, task/badge
brand colours — lives in `:root` custom properties, with a single dark
block overriding the values. Light-mode colours are unchanged.
- Declare `color-scheme: light dark` so native scrollbars and form
controls follow the active theme.
- Use the system monospace stack (`ui-monospace, ...`) via a `--mono`
variable, replacing the hardcoded font stacks.
- Dark tokens chosen to clear WCAG AA (>=4.5:1) for small text, and dim
brand colours (task-main, task-default) lifted for the dark surface.
Verified by rendering all three viewer pages in light and dark with a
headless browser: computed colours, contrast ratios, resolved theme
variables, and `color-scheme` asserted programmatically.
* fix(tool): resolve file_read paths against git top-level in monorepos
ocr review from a monorepo subdirectory failed with "file not found" (#287):
git reports diff and `git show HEAD:<path>` paths relative to the repo root,
but RepoDir was scoped to the invocation subdirectory, producing a double
prefix. resolveWorkingDir now anchors RepoDir at `git rev-parse
--show-toplevel` on the review path (requireGit=true); scan keeps the CWD so
its `git ls-files` walk stays scoped.
The top-level lookup uses a stdout-only git helper so stderr notices can't
pollute the path, and fails loudly if --show-toplevel errors or is empty
(e.g. a bare repo) instead of silently reusing the subdirectory. Adds
regression tests for the subdir hoist, the scan-path scoping, git-show
resolution of root-relative paths, and the bare-repo failure.
* docs(rules): document repo-root rule.json resolution in monorepos
Since #287 anchored RepoDir at the git top-level, ocr review from a
monorepo subdirectory loads the repo-root .opencodereview/rule.json
rather than a subdir-local one. Call out this user-visible behavior at
loadProjectRule so the scope change isn't a surprise (review feedback).
Add two structured fields, category and severity, to every review finding
so CI integrations can sort, group, filter, or gate builds without
re-parsing natural-language comment text.
- Tool schema (tools.json): add category/severity as enum-constrained,
required properties of code_comment. severity is limited to
critical/high/medium/low (info dropped, since LLMs struggle to
distinguish low from info).
- System prompt (task_template.json) is intentionally left untouched to
avoid the review-quality regression observed on the benchmark suite;
the tool schema alone drives field population.
- JSON output: category/severity are flat siblings of content/start_line,
omitted entirely when empty (backward compatible).
- CLI output: render an inline [category - severity] badge before the
comment, colored by severity.
- Sync docs across all five README locales.
* fix(tui): persist official-tab models and refine saved secret hint
Persist user-added models to providers.<name>.models on the official tab.
When an API key or auth token is already saved, show a replace hint with a
prefix/suffix fingerprint (skipped for short keys), use a fixed mask placeholder,
and ensure typing or paste replaces the saved value instead of re-saving it.
* fix(tui): model add/delete UX and config wizard hardening
- Add model add/delete in config provider and config model (official + custom)
- Show d Delete only on model rows; green highlight when selected
- Improve Esc cancel text; track savedInSession to avoid misleading messages
- Reload config on save failure; read registry models fresh after reload
- Export llm.ModelListContains; fix config model persist using registry-only check
* fix(tui): defer provider config until confirm and harden API key UX
Only persist provider/model on wizard confirm; keep in-session picks via
sessionModelPick. Validate API key before quit, clear saved keys when emptied,
and improve official env-var hints and custom edit clear behavior.
* feat(tui): show active model suffix on official provider list
Align Official tab with Custom: display (model) next to the active preset
when cfg.Provider matches and a global model is configured.
- Increase section height to prevent terminal bottom from being clipped by overflow:hidden
- Adjust terminal body bottom padding to 8px for better spacing
* fix(pages): address CodeQL XSS alerts in markdown rendering
Alert #4 (headingId.ts): replace the unreliable single-pass regex used to
strip HTML tags (incomplete multi-character sanitization) with DOMPurify.
This also fixes a pre-existing mismatch where headings containing HTML
entities produced different anchor ids on the TOC vs renderer sides.
Alert #5 (MarkdownRenderer.tsx): mermaid runs with securityLevel:'strict'
and already sanitizes its own SVG output, so re-running DOMPurify over the
whole SVG broke rendering (namespaces, inline <style>, foreignObject
labels). Make securityLevel explicit and inject mermaid's trusted output
directly, annotated with a codeql suppression comment.
* docs(pages): clarify CodeQL XSS suppression justification in MarkdownRenderer
The suppression comment claimed the mermaid SVG is 'not raw user input',
which understates the trust boundary. The SVG is in fact derived from
user-controlled mermaid code; safety relies on mermaid's securityLevel:
'strict' sanitizing the output via DOMPurify. Update the comment to state
this accurately and flag that the boundary depends on that setting.
Each docs chapter previously lived at the single /docs route driven by
component state, so chapters had no shareable URL and browser back/forward
did not work. Add a /docs/:slug route and derive activeSlug from the URL
param (falling back to quickstart), navigating via the router on switch.
Also add a dev-time invariant that throws when two sidebar entries share a
slug, since each slug now maps to exactly one URL.
Sync the reorganized zh/mcp.md into the English and Japanese versions:
remove the MCP client-side paragraph and the How it works section,
split Configuration into Adding/Removing subsections, drop the manual
JSON edit example, move env to the end of the code block and table, and
align dash spacing. Also fix a double-space typo in the zh version.
Add an MCP tutorial page (en/zh/ja) covering how OCR acts as an MCP
client that pulls tools from external MCP servers into a review:
configuration via mcp_servers, the config fields, CLI usage, tool
filtering, name conflicts, the setup command, and troubleshooting.
Wire the new page into the docs system (index.ts, DocsPage sidebar,
i18n en/zh/ja) and cross-link from the integrations page to clarify the
client vs server distinction.
Bump the Git prerequisite version hint from >= 2.38 to >= 2.41, and add
the Prerequisites section to the ja/ko/ru localized READMEs which were
missing it.
Translate all 17 docs pages from zh to ja under content/docs/ja/,
translate frontmatter titles, and wire ja into content/docs/index.ts
(replacing the previous English fallback). Sidebar i18n keys already
existed in i18n/ja.ts, so navigation renders Japanese automatically.
* feat(pages): add Docs page with search, markdown rendering, and i18n support
- Add DocsPage with full-text search modal (⌘K trigger)
- Add MarkdownRenderer with DOMPurify sanitization
- Add bilingual docs content (en/zh) for all sections
- Add shared headingId utility for consistent TOC anchors
- Add search keyboard hints with i18n support
- Update Navbar with Docs navigation link
- Add icon-search.svg asset
- Configure webpack for markdown imports
* fix(pages): address PR #273 code review feedback
- Replace marked.setOptions() with new Marked instance (no global mutation)
- Escape heading ID attribute value to prevent XSS
- Use crypto.randomUUID() for mermaid diagram IDs (no collisions)
- Add cancellation flag for async mermaid renders on unmount
- Move inline <pre> styles to CSS class (only dynamic align-items inline)
- Move @types/dompurify to devDependencies
- Remove @ts-nocheck from docs/index.ts
- Extract getRawContent helper to reduce duplication
- Fix searchDocs fallback consistency (add enDocs fallback)
- Fix heading ID mismatch by stripping markdown links before ID generation
- Separate sidebar chevron (expand) from label (navigate)
- Guard ⌘K shortcut against input/textarea focus interception
* fix(telemetry): support http:// scheme in otlp_endpoint for insecure gRPC
Parse the otlp_endpoint scheme before creating the OTLP gRPC exporters:
- http://host:port -> strip scheme, call WithInsecure() (plaintext gRPC)
- https://host:port -> strip scheme, keep default TLS
- host:port -> unchanged, keep default TLS (backward compatible)
Scheme matching is case-insensitive. Applies to both the trace and
metric exporters in initOTLPProviders.
Fixes#268
* fix(telemetry): trim trailing slash from otlp_endpoint after scheme strip
Addresses review feedback: a URL-style endpoint with a trailing slash
(e.g. "http://localhost:4317/") left the trailing "/" in the address
passed to WithEndpoint(), which expects a bare host:port with no path
and could cause connection failures.
* feat(llm): add z-ai-coding provider for GLM Coding Plan endpoint
Z.AI (智谱) subscribers to the GLM Coding Plan must route requests through
the dedicated coding endpoint (https://open.bigmodel.cn/api/coding/paas/v4)
for them to be billed against the subscription quota. The existing z-ai
provider points at the generic pay-as-you-go endpoint
(https://open.bigmodel.cn/api/paas/v4), so Coding Plan keys silently drain
the wallet balance instead of consuming the plan quota, surfacing as a
spurious "1113 余额不足" error even when the plan is barely used.
Add a dedicated z-ai-coding provider following the existing *-tokenplan
pattern (dashscope/dashscope-tokenplan, tencent-tokenhub/hy-tokenplan).
It reuses Z_AI_API_KEY — the same key authenticates against both endpoints,
so selecting this provider is all that's needed to activate the plan. The
model list is restricted to the models officially supported by the Coding
Plan to avoid selecting a non-plan model that falls back to wallet billing.
- internal/llm/providers.go: register z-ai-coding preset
- extensions/vscode/src/shared/providers.ts: mirror the preset (kept in
sync with the Go registry per the file header)
- internal/llm/providers_test.go: update the sorted provider list assertion
Co-Authored-By: Oz <oz-agent@warp.dev>
* docs(pages): add Z.AI GLM Coding Plan config tip to docs page
Subscribers to the Z.AI (Zhipu) GLM Coding Plan must route requests through
the dedicated coding endpoint (https://open.bigmodel.cn/api/coding/paas/v4)
to bill against the plan quota. The default z-ai preset points at the generic
pay-as-you-go endpoint, so coding-plan keys silently drain the wallet and
surface a spurious "1113 余额不足" error — a recurring trap for new users.
Add a provider-specific callout at the end of the Docs config section showing
the one-line fix that works today on any released version:
ocr config set providers.z-ai.url https://open.bigmodel.cn/api/coding/paas/v4
This complements the z-ai-coding provider added in the previous commit: the
provider gives a native first-class option going forward, while this doc tip
rescues users already running released builds. Copy/localized for zh/en/ja.
Co-Authored-By: Oz <oz-agent@warp.dev>
* fix(llm): address review feedback for z-ai-coding provider
- Switch z-ai-coding to a dedicated Z_AI_CODING_API_KEY env var instead
of reusing Z_AI_API_KEY, matching the tokenplan-provider convention so
pay-as-you-go and Coding Plan keys can be configured independently
- Remove comment blocks from the z-ai-coding presets (Go and TS) to keep
the registry as plain data consistent with the other entries
- Revert pages/ changes (i18n + DocsPage.tsx); provider docs are out of
scope for a provider-registration PR
Co-Authored-By: Oz <oz-agent@warp.dev>
---------
Co-authored-by: mountainwu <mountainwu@kuainiugroup.com>
Co-authored-by: Oz <oz-agent@warp.dev>
Move the 5-minute timeout note from the standalone mcpNote paragraph
into the setup field description in the MCP Server table, so users
see the timeout constraint directly alongside the field definition.
* docs(examples): add GitFlic CI auto-review example
Add examples/gitflic_ci/, a CI-layer integration that reviews GitFlic
merge requests with OpenCodeReview and posts the findings as MR
discussions. Like the GitHub and GitLab examples, the posting glue lives
outside the core binary.
post_review.py (standard library only) reads `ocr review --format json`
and posts inline discussions plus a fallback note and a summary. GitFlic's
Discussions API requires an old-side line for inline comments, which the
new-side-only review output lacks, so the script recomputes it from the
same merge-base diff the review ran on. Ships with a stdlib unittest suite
whose line-mapping cases are ported from the review's diff logic.
* docs(readme): list the GitFlic CI example in the localized READMEs
* fix(examples): address GitFlic CI review feedback from PR #201
Apply the five review comments left on the PR:
- gitflic-ci.yaml: guard `ocr config set llm.model` behind a non-empty
check so the documented-optional OCR_LLM_MODEL no longer breaks the
config step when it is unset
- gitflic-ci.yaml: skip posting when `ocr review` produced no output
(the step ends with `|| true`) instead of feeding empty/partial JSON
to post_review.py
- post_review.py: redact the token from HTTP error snippets so it cannot
leak into CI logs if GitFlic echoes the request back in an error body
- post_review.py: read the review-result file via a `with` block so the
handle is closed explicitly
- examples/README.md: add the missing trailing newline
A panic in a single file's review goroutine (dispatchSubtasks) or in a CommentWorkerPool task previously crashed the whole ocr process. Recover in both: the per-file panic is isolated like an error return (counted in subtaskFailed + recorded as a subtask_error warning with stack trace + telemetry, using the parent ctx since fileCtx is already cancelled on unwind), and a panicking comment-pool task is contained so healthy tasks still complete.
Rebased onto current main: the pool moved to internal/llmloop, so the pool-side recover + the panic-isolation test now live in internal/llmloop/pool.go and pool_test.go; the per-file recover stays in internal/agent/agent.go. Also documents CommentWorkerPool.Await's concurrency contract (Submit must not race Await).
Update documentation strings across en/ja/zh to reflect the new
provider-based configuration: add full scan review mode, list built-in
providers (Anthropic, OpenAI, DashScope, DeepSeek, Z.AI), and replace
legacy config keys with provider-scoped keys including extraHeaders.
* fix(ci): add idempotency check to prevent duplicate review posts on retry
When the batch createReview fails with a 5xx/408/network error, the
request may still have landed on the server. Before retrying per-comment,
the workflow now:
- Tags each review/comment/summary with a per-run HTML comment ID derived
from runId + runAttempt + content hash.
- Queries existing reviews and review comments to detect whether the batch
actually landed, and only retries the comments that are missing.
- Before retrying an individual comment whose request may have reached
GitHub, cools down (honoring rate-limit headers) then checks whether the
comment already exists, treating it as success instead of posting a
duplicate.
- Skips posting the summary comment when one with the same run tag already
exists.
- Adds read-API retry/pacing helpers (withRetry/readWithPacing/readAllPages)
with shorter spacing than writes (OCR_READ_SUCCESS_DELAY /
OCR_READ_LOW_REMAINING_SPACING) since reads are cheaper but still consume
the primary rate limit.
Degrades gracefully to the original fallback (accepting duplicate risk)
when the idempotency read calls themselves fail.
* fix(ci): harden idempotency checks in ocr-review workflow
Address code review findings on the GitHub Actions PR auto-review
workflow (applied to both .github/workflows and examples copies):
- readAllPages: cap pagination at maxPages=50 (default) to prevent
unbounded loops, and validate the argument is a positive integer.
- getPostedCommentIds: anchor the ID regex to the HTML comment wrapper
(<!-- ocr-... -->) with a capture group to avoid false positives from
user-generated content.
- isCommentAlreadyPosted: return null (unknown) instead of false when
the read API fails, so callers do not silently risk duplicates; accept
a postedIdsCache to reuse a single paginated walk across retries.
- hasIssueCommentWithId: return null (unknown) on read API failure, and
match the summary tag with an anchored regex for consistency.
- Call sites: handle null by skipping retry/posting to avoid duplicates
while surfacing the failure in the summary.
* fix(ci): validate env config and document intentional behaviors
Address code review findings on the ocr-review workflow (applied to
both .github/workflows and examples copies):
- parseNonNegInt: add a validation helper for env-var parsing so
negative or non-numeric values (e.g. OCR_MAX_RETRIES=-5) fall back to
defaults instead of bypassing the `|| default` guard (a negative
parseInt result is truthy). All seven retry/pacing config values now
use it.
- readAllPages: document that the 50-page cap is an intentional safety
valve against unbounded loops, not a normal mode; callers that depend
on completeness already degrade safely to null (unknown), so a
truncated walk does not silently produce duplicates.
- commentId: document that the 12-hex-char (48-bit) hash collision
scope is a single PR (listReviewComments is PR-scoped) and a single
run produces only tens to hundreds of comments, making the
birthday-bound collision probability negligible (~1e-7 at 10k).
* docs(github_actions): sync README with retry/idempotency features in ocr-review.yml
- Add OCR_READ_SUCCESS_DELAY and OCR_READ_LOW_REMAINING_SPACING variables
for read API pacing used by the idempotency check
- Document the three GitHub rate-limit retry strategies (primary reset,
retry-after header, secondary no-header backoff)
- Add 'Idempotency: avoiding duplicate review comments' section describing
how the workflow detects already-landed comments via per-run HTML tags
and skips retrying when the read API is unavailable
* fix(ci): use full sha256 hash for review comment idempotency IDs
Drop the .slice(0, 12) truncation in commentId() and use the full 64-char
(256-bit) sha256 hex digest. The truncated 12-char hash carried a tiny but
nonzero collision risk whose failure mode was a silently dropped inline
comment (the idempotency check would mistake two distinct comments for
duplicates). The full hash makes the collision probability effectively
zero with no meaningful downside; the ID regex already used [a-f0-9]+ so
it accepts the longer IDs unchanged.
* fix(ci): use random per-comment IDs and defer body assembly in review workflow
Replace the content-derived commentId() (sha256 of path/line/content) with
a random per-comment ID (crypto.randomBytes) and restructure the inline-
comment flow around an item struct that carries { comment, id, lines }.
This fixes two issues in the idempotency check:
1. ID was recomputed on every failure check. Each inline comment is now
assigned one random ID up front and carried on the item struct, so the
retry/idempotency logic reads item.id directly. The comment body (which
embeds the ID) is assembled only at API-call time in toReviewPayload(),
eliminating repeated hash computation.
2. Content-derived IDs collided for distinct comments sharing the same
path/line/content. A random ID guarantees two such comments get
different IDs, so the idempotency check no longer mistakes the second
for a duplicate of the first and silently drops it on retry.
formatComment/commentId are removed (no callers remain) and replaced with
newCommentId/resolveLines/toReviewPayload/buildBody. The matching regex
already used [a-f0-9]+ so it accepts the new random tokens unchanged.
README ID-format placeholder updated from <hash> to <token>.
* docs(ci): correct misleading readAllPages truncation comment
The comment claimed 'a truncated walk does not silently produce
duplicates' because callers 'degrade safely by returning null on read
failures.' That reasoning only holds when the read API THROWS (rate
limit, 5xx): isCommentAlreadyPosted/hasIssueCommentWithId then return
null (unknown) and the caller skips retrying. A truncated walk does not
throw — it returns a partial set silently, so isCommentAlreadyPosted
returns false (definitively 'not posted') for comments beyond the cap,
and the retry loop reposts them, producing duplicates.
Rewrite the comment to state the cap is an intentional safety valve and
to explicitly distinguish truncation (partial data, can duplicate) from
thrown read failures (null/unknown, safe). No behavior change.
* fix(ci): drop stale postedIdsCache to prevent duplicate inline comments
isCommentAlreadyPosted reused a single listReviewComments snapshot
(postedIdsCache) across all per-comment retries. As comments landed
during the loop, the snapshot went stale; a 5xx-landed comment checked
against the stale snapshot would be reported as 'not posted' and
retried, posting a duplicate.
Remove the cache and walk fresh on every check. The extra reads are
paced via readAllPages/readWithPacing (with retry honoring retry-after
and x-ratelimit-reset) and degrade to null — skip retry — if the read
API ultimately fails, so they cannot produce duplicates. The cache
provided no real benefit in this path: checked comments are either
genuine misses (correctly false) or just-landed (a fresh walk catches
them), so hits essentially never occurred.
