Skip to content

feat(reviewer): enable automatic review by default with visible session controls - #353

Merged
sciencediscovery-bot[bot] merged 4 commits into
openJiuwen-ai:mainfrom
zy95-12:agent/issue-323
Oct 17, 2026
Merged

sciencediscovery-bot[bot] merged 4 commits into
openJiuwen-ai:mainfrom
zy95-12:agent/issue-323

Conversation

@zy95-12

@zy95-12 zy95-12 commented Oct 16, 2026 •

Copy link
Copy Markdown
Collaborator

New sessions enabled automatic review while the global Reviewer capability defaulted off, so eligible reports could silently miss verification. This enables the capability for fresh catalogs (and catalogs without the field), preserves saved opt-outs, and exposes Automatic review beside Approval mode. Eligible reports use the existing background audit path without another user message.

Fixes #323.

Changes

  • Keep Quick/Deep configuration and manual Run review in the workspace.
  • Show a composer notice when the global capability prevents automatic review, with access to Specialist settings.
  • Persist an explicit skipped reason when the capability or session switch prevents an already-queued audit. Keep the terminal storage status failed so an unexecuted review is never reported as passed.
  • Display these intentional automatic skips as neutral conversation records, with the reason in the collapsed heading and expanded body. Preserve suppression of stale-artifact failures and empty legacy checkpoints; manual failures remain visible.
  • Cover defaults, migration, saved opt-outs, automatic execution, and both skip reasons in regression tests. Extend the browser journey for toggle persistence, automatic results, manual review, and a disabled-capability skip record that survives reload.
  • Update English and Chinese documentation to identify the live controls and distinguish deprecated fields.

Automatic review remains read-only background work with the existing batching window; it does not delay the main response or automatically revise reports.

Validation

Current follow-up commit: 7a57f60e. All local build/test commands used the cooperative resource lock.

  • pnpm typecheck: passed.
  • pnpm ci:l1: planned 4 / executed 4 / passed 4.
  • pnpm ci:st: planned 3 / executed 3 / passed 3.
  • git diff --check: passed.
  • pnpm ci:ut: initial setup failed because the default uv cache was read-only. Retried with a writable task cache and one UT worker: planned 3819 / executed 0 / passed 0; sandbox/non-root preflight blocked execution.
  • pnpm ci:e2e: planned 74 / executed 0 / passed 0; preflight failed with BUBBLEWRAP_UNAVAILABLE.

Previous head 7833a87a passed UT, ST, L1 and three mocked browser shards in run 38936603611. Its standard-config shard ran 21 cases: 20 passed, 1 failed. The automatic-review journey incorrectly asserted visibility of the <small> subtitle, which collapsed process-record CSS intentionally hides. The skip reason is already rendered in the visible <strong> heading. This follow-up corrects that selector and checks heading visibility before and after reload, neutral status, and the full reason after expansion. No product behavior or CI gate changed in this follow-up.

Earlier panel regression tests at 7833a87a: 16 passed, 0 failed, 0 skipped. These historical results do not certify the new head.

E2E user journeys

BLOCKED locally at 7a57f60e: Bubblewrap is unavailable, so no browser journey executed. The journey covers automatic review without another user message, session toggle persistence, manual review, and a capability-off skip record before/after reload and after expansion.

Current-head CI run 38939966736 completed successfully at 7a57f60e7c171dded0e66aeeeec2fd1f159f884f. Verified from the run logs: L1 planned/executed/passed 4/4/4, UT 3819/3819/3819, ST 3/3/3, and mocked browser E2E 74/74/74 (standard-config 21, standard-journeys 27, research 20, literature 6), with 0 failed and 0 skipped in each executed slice. The corrected automatic-review browser journey passed within standard-config. Coverage and merge-gate also succeeded. Real-model and distribution jobs were skipped as expected for the PR profile. Local sandbox limitations above remain; remote current-head validation now supplies the missing UT/browser evidence. No gates were bypassed and no paid real-model tests were run. The prior review thread is resolved, with no open findings in the latest review. The PR is Ready and approved. As of 2026-10-17, its merge-queue entry is AWAITING_CHECKS; auto-merge is disabled. Merge-group validation remains pending.

Release

Release category: release:feature

User impact: Automatic review works by default for fresh catalogs, is discoverable beside the composer, and capability/session-blocked queued reviews have visible reasons.

Compatibility and migration: Existing saved opt-outs are preserved; catalogs without a Reviewer switch acquire the enabled default. No schema migration or legacy-switch reinterpretation.

@sciencediscovery-bot sciencediscovery-bot Bot added the enhancement New feature or request label Oct 16, 2026

@sciencediscovery-bot sciencediscovery-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Round 1 inline notes for draft tip b8c0c086. See the upserted summary comment for the full verdict.

Comment thread services/api/src/reviewer-specialist/audit-coordinator.ts
@sciencediscovery-bot

sciencediscovery-bot Bot commented Oct 16, 2026 •

Copy link
Copy Markdown

This is an automated review note from the repository maintenance bot (AI-assisted). Maintainers make the final call.

Last updated: 2026-10-17 14:07 CST (merged / head 7a57f60e)

Status: Merged into main on 2026-10-17 14:07 CST through the merge queue (merge commit 74e248d8), after the bot enabled merge when ready on the Round 4 reviewed head. The merged head 7a57f60e is the same tip Round 4 reviewed, so the verdict below still describes what landed. This bot does not judge CI.

Verdict

No clear defects found in this round. Reviewed head 7a57f60e against main 42e3a9aa (18 files, +203/−43; 4 commits). Standalone tip review of the whole three-dot diff, re-done now that the PR is Ready (not delta-only vs Round 3).

The tip enables Reviewer Specialist by default for fresh catalogs and catalogs missing the field (?? true), preserves explicit false opt-outs, keeps session automatic review on by default, exposes Automatic review beside Approval mode, shows a composer notice when the system capability is off, persists terminal skip reasons on blocked queued automatic audits, and surfaces those intentional skips as neutral conversation panels. EN/ZH docs and store/coordinator/panel/journey coverage were updated accordingly. The latest commit strengthens the journey assertion so the skip reason is checked on the visible collapsed heading and again in the expanded body.

What I checked at this tip:

  • Defaults / migration. emptyCatalog sets reviewerSpecialistEnabled: true; load uses saved.reviewerSpecialistEnabled ?? true. Explicit false survives reload (store test + disabled-reload assertion). Catalogs that omit the field migrate to true. Session create still seeds reviewerAutomaticReviewEnabled: true.
  • Enqueue vs execute. Global capability is not an early enqueue veto; session opt-out still skips scheduling. When a queued automatic task hits a disabled capability or session switch, the coordinator publishes a checkpoint, marks the task failed with an explicit Automatic review skipped:… string, and updates the checkpoint message — an unexecuted review is never left looking like a pass.
  • Conversation UI (Round 1 fix, still in place). ReviewerPanel.tsx detects automaticSkip for automatic-review:* + failed + skip-prefixed error: shows a neutral process-record panel (no failed class) with the reason in heading/body; stale-artifact and empty legacy automatic failures remain hidden; manual failures still render as failed. Panel unit tests cover both skip strings and the stale hide case.
  • Journey skip visibility. Collapsed .process-record panels hide small/subtitle via CSS; the tip journey now asserts the skip reason on .reviewer-specialist-panel-heading strong (which embeds Reviewer Specialist · {reason}), keeps the panel non-failed across reload, and after expanding summary checks .reviewer-specialist-results for the exact reason body. Matches the panel markup (strong carries the summary when not running; expanded body renders <p>{checkpointError}</p>). No false expectation introduced.
  • Composer controls. Toggle reflects enabled && session.automaticReviewEnabled; when the specialist is off, click opens Specialists settings and the status notice uses reviewer.automaticUnavailable (EN + zh-CN). Separate .automatic-review-toggle shares Approval-mode styling.
  • Docs. Live controls vs deprecated reviewMode / semanticReviewEnabled called out in EN/ZH core and runtime-behavior pages; skip-card wording matches UI.

Findings

None open at this head.

Fixed since Round 1

  • Inline on services/api/src/reviewer-specialist/audit-coordinator.ts (comment 4297029842): automatic skip/fail checkpoints were invisible because ReviewerPanel hid all non-running empty automatic-review:* panels. Fixed in 7833a87a via neutral skip rendering; still fixed at 7a57f60e. Confirmed earlier on that thread (no further reply this round — status unchanged).

Notes (not blocking)

  • Behaviour to confirm (not a defect): because the global capability is no longer an enqueue veto in audit-coordinator.ts, a workspace that explicitly turned Reviewer Specialist off but keeps the per-session switch on will get a neutral skip record for every eligible report. This matches the PR description and the journey test; maintainers may just want to confirm that repeated skip records are the intended UX for long-term opt-outs.
  • Catalogs that never persisted reviewerSpecialistEnabled will acquire true on next load — intentional per PR/docs; worth a release-note mention for operators who relied on the old implicit off.

Linked Issue

Fixes #323: substantially addressed for the 2026-10-16 collaborator product decisions (Claude Science–aligned defaults, composer-adjacent toggle, same-workflow automatic path, visible capability-off notice, and visible skip records for blocked queued audits).

  • Met: auto-review on by default for new / field-missing catalogs; explicit opt-outs preserved; session toggle beside Approval mode; eligible reports enqueue without a second user message; composer explains when the specialist is off; skip reason persisted and shown as a neutral conversation panel; docs distinguish live switches from legacy fields; journey now asserts the visible heading and expanded body for the skip card.

Merge status: #323 is now closed (completed) via this PR's Fixes #323. The two non-blocking notes below remain for maintainers' awareness (the author has already given dispositions on them).

Labels

Inline

No new inline comments this round. Prior Round 1 finding remains fixed; no thread reply (status unchanged).

Cleanup

The bot's review worktree for this PR was removed after merge. No timeline ping for merge cleanup.

@sciencediscovery-bot

Copy link
Copy Markdown

Round 1 — synchronize draft review at b8c0c086 (Fixes #323 substantially; one UI visibility gap on automatic skip cards). Summary: see the bot’s upserted <!-- repo-bot:summary --> comment on this PR.

@sciencediscovery-bot

Copy link
Copy Markdown

Round 2 — draft review at 7833a87a: no clear defects found; the Round 1 skip-visibility finding is fixed and #323 now looks fully addressed. Details: #353 (comment)

@sciencediscovery-bot

Copy link
Copy Markdown

Round 2 — synchronize draft review at 7833a87a: no clear defects found; Round 1 automatic-skip visibility finding verified fixed (confirm reply on the inline thread). Full tip verdict: see the bot’s upserted <!-- repo-bot:summary --> comment on this PR. Fixes #323 still substantially addressed; release:feature kept (author).

@sciencediscovery-bot

Copy link
Copy Markdown

Round 3 (synchronize / draft) at 7a57f60e: no clear defects — tip still enables automatic review by default with visible skip panels; journey now asserts the visible skip heading. Full review.

@sciencediscovery-bot

Copy link
Copy Markdown

Round 4 (ready for review) at head 7a57f60e: full review of the tip — no clear defects found in this round; one behaviour note for maintainers to confirm (repeated skip records for workspaces that keep Reviewer Specialist off). Details in the summary comment.

@zhaozhaozz zhaozhaozz left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Approving. An explicit opt-out stays off, and a skipped automatic review is shown as a skip rather than a pass.

@sciencediscovery-bot
sciencediscovery-bot Bot added this pull request to the merge queue Oct 17, 2026
@zy95-12

zy95-12 commented Oct 17, 2026

Copy link
Copy Markdown
Collaborator Author

Disposition of the two non-blocking notes in Round 4:

  • Repeated capability-off skips: retain the implemented behavior. The accepted direction, points 5–6 requires a visible outcome when session automatic review is on and allows the system capability gate. At 7a57f60e, eligible reports therefore continue to produce neutral skip records while the capability is off; existing batching/deduplication still applies, so this is not necessarily one card per artifact. This is an implementation consequence of that accepted scope, not a new product decision. Silently suppressing subsequent blocked audits would remove their outcome. A session opt-out prevents new scheduling; the disabled capability never executes the reviewer. See coordinator logic, disabled-capability regression, and browser skip/reload assertions. No suppression policy is added in this maintenance pass.
  • Migration release note: already represented in the release input. The PR's Release / Compatibility and migration paragraph explicitly says saved opt-outs are preserved and catalogs without the switch acquire the enabled default; release:feature is the sole release label. The migration and saved-false tests cover both cases. Keep that compatibility text when preparing the release note; no release is published by this worker.

Rechecked current-head run 38939966736 and its logs: L1 4/4, UT 3819/3819, ST 3/3, mocked E2E 74/74; zero failures/skips in those slices. No new code or tests were needed for these dispositions. The previous visibility finding and duplicate confirmation replies are already addressed by 7833a87a and the resolved source thread.

Corrected the stale Draft sentence in the PR body: currently Ready, approved, CLEAN, and actually queued as AWAITING_CHECKS; auto-merge is disabled. Merge-group checks remain pending. No merge/enqueue command was issued.

Merged via the queue into openJiuwen-ai:main with commit 74e248d Oct 17, 2026
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request release:feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Usability: auto-run review verification when needed (don’t require a second user trigger)

2 participants