feat: [ENG-1593] add brv review --disable / --enable toggle#563
feat: [ENG-1593] add brv review --disable / --enable toggle#563RyanNg1403 wants to merge 3 commits intomainfrom
Conversation
Adds a project-scoped HITL review toggle. When disabled: - `brv curate` no longer prints the "X operations require review" prompt - `--detach` curate-log entries omit the per-op review marker - `brv dream` no longer surfaces its own needsReview ops as pending reviews - `brv review pending` lists nothing new until re-enabled - Review backups (the rollback substrate) are not written Single source of truth: the daemon snapshots `reviewDisabled` once at the task-create boundary and forwards it through TaskInfo (daemon-side, used by CurateLogHandler) and TaskExecuteSchema (wire format, opened as an AsyncLocalStorage scope around the agent-process task body so any descendant async callsite — direct curate-tool invocation OR sandbox `tools.curate(...)` via CurateService — observes the same value even if the user toggles mid-task). Same propagation pattern as CurateResultCollector. Mid-task toggle race coverage: - Started disabled, toggle enable → daemon snapshot held disabled → no orphan backup, no pending entry - Started enabled, toggle disable → daemon snapshot held enabled → backup created, pending entry created - Verified across 24 curates + 6 dreams in interactive testing. Top-level `brv review` command (and the new `--enable` / `--disable` flags) routes through the daemon transport via two new events (`review:getDisabled`, `review:setDisabled`) — same wiring pattern as `brv review pending` / `approve` / `reject`. Existing review subcommands are untouched.
RyanNg1403
requested review from
bao-byterover,
cuongdo-byterover,
danhdoan,
leehpham and
ngduyanhece
as code owners
|
Claude finished @RyanNg1403's task in 5m 35s —— View job PR Review: feat: [ENG-1593] add brv review --disable / --enable toggleTasks
SummaryThe design is solid. The snapshot-at-task-create approach (one ALS scope per One correctness bug found. Three style/standards comments posted inline. Issues FoundBug — idle-triggered dreams ignore the toggleInline comment on The Fix: stamp This gap isn't covered by the existing test suite: Standards violation —
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 96cdaf680a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ping and curate-tool style Fix #1 (blocker — brv-server.ts): the idle-trigger dream-dispatch path called `agentPool.submitTask(...)` directly, bypassing `TaskRouter.handleTaskCreate` and therefore the daemon's reviewDisabled snapshot. Without the stamp, `task.reviewDisabled` arrived undefined in the agent process, the AsyncLocalStorage scope was never opened, and `DreamExecutor` defaulted to review-enabled — so idle dreams ignored the project's `reviewDisabled` setting regardless of `brv review --disable`. Hoisted the `ProjectConfigStore` declaration up the file and added a shared `resolveReviewDisabled` resolver that both the idle-dream dispatch and the existing `TransportHandlers.isReviewDisabled` reuse. The two paths now stamp identical values, so review semantics are consistent across CLI dispatch and idle-trigger dispatch. Fix #2 (curate-tool.ts): replaced `JSON.parse(raw) as {reviewDisabled?: unknown}` in `isReviewDisabledForBrvDir` with a proper `hasReviewDisabledField` type guard. CLAUDE.md forbids `as Type` assertions; the new guard narrows safely without an escape-hatch cast. Fix #3 (curate-tool.ts): dropped redundant braces around the single-expression arrow body in the `onAfterWrite` definition. The union-type ordering nit (`undefined | WriteCallback` vs reverse) was left as-is — `perfectionist/sort-union-types` requires the existing order, so the reviewer's suggested swap would actually have introduced a lint error.
PR Review: feat: [ENG-1593] add brv review --disable / --enable toggleTasks
Working… |
P2 fix (task-router.ts): `snapshotReviewDisabled` returned `undefined` when the resolver threw, which re-introduced the exact daemon/agent divergence the snapshot is meant to prevent — daemon stamps no field → `CurateLogHandler` treats it as enabled (`?? false`), while the agent process opens no ALS scope and may observe a different value from `.brv/config.json` in the curate-tool fallback. Now stamps an explicit `false` (fail-open: review enabled) on resolver throw, so both sides observe a single concrete value. Aligns with the agent-side `isReviewDisabledForBrvDir` which also fails open. `undefined` is still returned when no resolver is wired or no projectPath was resolved — those are legitimate "not configured" cases that downstream consumers handle correctly. Adds a regression test pinning the new behavior in `reviewDisabled stamping` describe block.
|
Claude finished @RyanNg1403's task in 5m 42s —— View job PR Review: feat: [ENG-1593] add brv review --disable / --enable toggleTasks
Feedback Addressed Since Round 1All four issues from the previous review pass are resolved in commits
The New Inline CommentsFive inline comments posted (see diff):
What Works Well
Overall Score: 5 / 5The implementation is ready to merge. All blocker and standards issues from round 1 are resolved. The design is sound, the code is clean, and the tests cover the critical paths. The two remaining nitpicks ( |
1 participant
Summary
brv curatealways emitted the "X operations require review" prompt, dream surfaced its own needsReview ops, and curate-log entries always carried review markers — even for projects/workflows where review is unwanted.reviewDisabledflag onBrvConfig. New top-levelbrv reviewcommand exposes--enable/--disableflags and a bare-form status read; both routed through new daemon transport events (review:getDisabled,review:setDisabled). The daemon snapshotsreviewDisabledonce at the task-create boundary and forwards it throughTaskInfo(daemon-side, used byCurateLogHandler) andTaskExecuteSchema(wire format, opened as anAsyncLocalStoragescope around the agent-process task body so any descendant async callsite — direct curate-tool invocation OR sandboxtools.curate(...)viaCurateService— observes the same value). Dream-side review entry creation and review-backup writing both honor the same snapshot.brv review pending/approve/rejectsubcommands and their wiring are untouched.brv initflow is untouched —reviewDisabledis optional and defaults to enabled (fail-open). No global / machine-wide override; the toggle is strictly project-level.Type of change
Scope (select all touched areas)
Linked issues
main)Root cause (bug fixes only, otherwise write
N/A)N/A — feature add. (Mid-task snapshot race surfaced during the work; closed by the AsyncLocalStorage design from day one of the implementation, no separate fix.)
Test plan
test/commands/review-toggle.test.ts(new) — top-level command surface (10 tests, transport-stubbed, mirrors peertest/commands/review.test.ts)test/unit/infra/transport/handlers/review-handler.test.ts(new) —handleGetDisabled+handleSetDisabled(7 tests including "preserves other config fields")test/unit/agent/tools/curate-tool.test.ts— added 4 ALS-path tests (scope=true overrides config=false; scope=false overrides config=true; ALS viaexecuteCuratewith no_context.taskId— the regression that the prior Map-based registry missed; outside-scope falls back to file)test/unit/infra/process/curate-log-handler.test.ts— rewritten aroundtask.reviewDisabled(snapshot-at-create test included)test/unit/infra/process/task-router.test.ts— addeddescribe('reviewDisabled stamping')(4 tests)test/unit/infra/executor/dream-executor.test.ts— 6 dream-side tests including snapshot consistency betweenrunOperationsandcreateReviewEntries(prototype-patched)test/unit/core/domain/entities/brv-config.test.ts—reviewDisabledfield round-trip--enable/--disable(writing the same value succeeds silently)User-visible changes
brv review(no flags) — prints "Review log is enabled." or "Review log is disabled." (or{"reviewDisabled":<bool>}with--format json)brv review --disable— disables for the current project; prints "Review log disabled. To re-enable: brv review --enable"brv review --enable— re-enables; prints "Review log enabled. To disable: brv review --disable"brv review pending/approve/rejectsubcommands continue to work (no change)reviewDisabled?: booleanin<project_root>/.brv/config.json. Defaults to enabled (omitted = enabled). Per-project; not global, not per-worktree (worktrees inherit parent project's setting via the existing pointer model).Evidence
notes/eng-1593-review-disable/round3-cli-suite/PLAN.md— matrixnotes/eng-1593-review-disable/round3-cli-suite/RESULTS.md— 24 curates + 6 dreams; every mid-toggle observed the daemon snapshot, not the post-toggle confignotes/eng-1593-review-disable/IMPLEMENTATION_REPORT.md— full report including Implementation Decisions section per CLAUDE.mdgit diffis clean ofeng-1593-debugstrings)Checklist
npm test— 6920 passing)npm run lint— clean on changed files; pre-existing submodule lint error inpackages/byterover-packages/is unrelated and tracked separately)npm run typecheck)npm run build)feat: [ENG-1593] ..., noCo-Authored-By)reviewDisableddefaults to enabled / undefined)main(rebased to3c7f749cbefore commit)Risks and mitigations
CurateResultCollector(cited as precedent), and that has shipped reliably. Documented in the JSDoc ofcurate-tool-task-context.tsso future refactors don't break it silently.--disable/--enable(running the same flag twice in a row) doesn't differentiate between "actually toggled" and "already in that state" — both produce the same confirmation text.changedboolean for the CLI to branch on..brv/config.json— undefinedreviewDisabledis treated as enabled rather than disabled.snapshotReviewDisabledJSDoc andisReviewDisabledForBrvDirfallback.