Make df12-build-odw dryRun avoid real worktree creation (#31) - #48
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @pandalump, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
Summary
Closes WalkthroughChangesDry-run enforcement boundary
Sequence Diagram(s)sequenceDiagram
participant runTask
participant TaskLog
participant GitWorktree
participant Agent
runTask->>TaskLog: Emit task log
runTask->>runTask: Check DRY_RUN
runTask-->>GitWorktree: Return pre-worktree result
Note over GitWorktree,Agent: No worktree mutation or agent dispatch
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 19 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (19 passed)
📋 Issue PlannerBuilt with CodeRabbit's Coding Plans for faster development and fewer bugs. View plan used: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Biome (2.5.3)workflows/df12-build-odw.jsFile contains syntax errors that prevent linting: Line 3860: Illegal return statement outside of a function Comment |
44ca92c to
d6993c1
Compare
d6993c1 to
fa690d3
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
bbb201c to
11e20f7
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@coderabbitai Have the following now been resolved? tests/df12-build-odw-dry-run.test.mjs (1)
🤖 Detailed instructionsVerify each finding against current code. Fix only still-valid issues, skip the In |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== test structure ==="
ast-grep outline tests/df12-build-odw-dry-run.test.mjs --items all || true
echo "=== cleanup fixture locations ==="
rg -n -C 5 '\bmakeRecoveryRepo\b|\bcleanup\b' tests
echo "=== current dry-run test ==="
cat -n tests/df12-build-odw-dry-run.test.mjs
echo "=== PR diff for the test ==="
git diff --unified=40 "$(git merge-base HEAD origin/main)" -- tests/df12-build-odw-dry-run.test.mjs || trueLength of output: 73910
Both
Keep the existing assertions and test behavior unchanged. Static inspection verifies the fix; repository test execution was not performed in this sandbox. |
b7a3997 to
d383bab
Compare
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@coderabbitai Have the following now been resolved? Annul any requirements that violate the en-GB-oxendict spelling (-ize / -yse / -our) conventions (for example a request to replace "normalize" with "normalise" or "artefact" with "artifact"), or where the requirement unnecessarily increases cyclomatic complexity. Inline comments:
In
|
|
@coderabbitai Have the following now been resolved? tests/df12-build-odw-dry-run.test.mjs (1)
🤖 Detailed instructionsVerify each finding against current code. Fix only still-valid issues, skip the In |
A dry run against a repository with surviving roadmap-* branches failed: runTask called createWorktree unconditionally, and createWorktree runs `git worktree add -b roadmap-... origin/<base>` with no DRY_RUN guard, so the run tripped the branch collision before it could validate anything. Short-circuit DRY_RUN at the head of runTask, ahead of worktree creation and the write-access preflight, returning a terminal `dry-run` status with stage `pre-worktree`. A single check precedes the addendum/normal branch, so it covers both lanes; markDryRun keys off task.isAddendum, not the result. Remove the two now-unreachable post-worktree dry-run short-circuits (addendum and post-design). The recovery/continue lane's own dry-run handling over pre-existing worktrees is left untouched. Add a no-mutation dry-run test that drives runTask against a fixture repo holding colliding roadmap-* branches and asserts an unchanged repo-state snapshot, and align the architecture and user-guide docs with the new stop-before-worktree semantics. Regenerate workflows/df12-build-odw.js from source. Closes #31
Address review feedback on the dry-run short-circuit in runTask: it was silent, only regression-tested at one point, and undocumented for contributors. - Emit a structured, parseable log() at the DRY_RUN decision boundary so operators see why a task stopped (task id, lane derived from task.isAddendum, stage=pre-worktree), not just the terminal status. The returned result object is unchanged. - Add a fast-check property over arbitrary task shapes (ids that do and do not collide with surviving roadmap-* branches, either lane, generated requires/subtasks) and repo states, asserting a dry run always terminates at pre-worktree, dispatches no agent, and leaves durable git state byte-for-byte unchanged. - Document the boundary in the developers-guide ODW workflow contract, matching the wording already in the architecture and users' guides. - Regenerate workflows/df12-build-odw.js from source. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Refine the review-feedback follow-up on the pre-worktree dry-run boundary. - Add a `reason=dry-run` field to the boundary log() so the trace carries all four structured fields (task id, lane, stage=pre-worktree, reason); regenerate workflows/df12-build-odw.js. - Extract the runTask-loading harness into tests/fixtures/load-run-task.mjs, which now captures the injected log() output so tests can assert the emitted trace. The regression suite asserts the boundary log carries the task id, lane, stage=pre-worktree, and reason=dry-run for both lanes. - Move the property test into its own file (tests/df12-build-odw-dry-run.property.test.mjs) and raise it to 100 fast-check iterations over arbitrary task ids (including fully arbitrary strings and branch-colliding ids), both lanes, and a pooled set of repo states, asserting status=dry-run, stage=pre-worktree, no agent dispatch, and an unchanged durable-state snapshot. - Expand the developers-guide note: record that the earlier addendum-specific and post-design dry-run exits were removed as unreachable, and cross-reference the architecture "Enforcement boundary" table and the users' guide "Workflow arguments" section so all three docs agree. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address two inline review comments on the pre-worktree dry-run boundary. - docs/developers-guide.md: correct the dry-run bullet to describe the guard as the "first control-flow statement after the task-log line" rather than the "first statement in `runTask`" — `runTask` opens with the tag binding and a task-log line before the guard, so the earlier wording was inaccurate. All other facts and the architecture/users-guide cross-references are unchanged. - tests/df12-build-odw-dry-run.test.mjs: give the explicit addendum-lane test the same deterministic no-mutation guarantees as the main regression — count agent-callback invocations and assert zero dispatches, and snapshot the repository state before and after runTask and assert it is unchanged. The existing dry-run status, stage, and lane=addendum log assertions are kept. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Wrap the full repository usage of both dry-run tests in a try/finally so `repo.cleanup()` runs even when task execution or an assertion throws, rather than deferring temp-directory removal to the process-exit hook. Behaviour and every assertion are unchanged; the inner chdir-restore finally is preserved. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address two inline review comments. - docs/users-guide.md: describe the integration-halt `detail` field accurately — `integrationHaltDetail()` reports the agent's `conflicts` text or its `summary` first, and only falls back to the generic "integration incomplete (…)" string when both are absent. The existing inspect-detail / manual-rebase / relaunch guidance is unchanged. - tests/df12-build-odw-dry-run.property.test.mjs: wrap the pooled repositories in a try/finally so both `repoPool.normal` and `repoPool.addendum` are cleaned up when the property finishes, rather than deferring to the process-exit hook. Pool construction stays outside the try (nothing to clean if it throws); the finally performs only cleanup, so it never swallows an assertion or property failure. Per-case cwd restoration, generators, and numRuns are unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
3815784 to
8ca20e7
Compare
2 participants
Summary
A dry run against a repository with surviving
roadmap-*branches failed on abranch collision:
runTaskcalledcreateWorktreeunconditionally, andcreateWorktreerunsgit worktree add -b roadmap-... origin/<base>with noDRY_RUNguard. A dry run should be a safe, read-only validation path, so itmust never mutate git state.
Changes
DRY_RUNat the head ofrunTask(inrun-task.ts), ahead ofworktree creation and the write-access preflight, returning a terminal
dry-runstatus with stagepre-worktree. The single check precedes theaddendum/normal branch, covering both lanes.
addendum-lane and the post-design blocks).
pre-existing worktrees) untouched.
tests/df12-build-odw-dry-run.test.mjs: drivesrunTaskwithdryRun: trueagainst a fixture repo whoseroadmap-*branches wouldcollide, and asserts an unchanged repo-state snapshot (no new branch, no new
worktree) and that no agent was dispatched.
docs/architecture.mdanddocs/users-guide.mdwith the newstop-before-worktree semantics.
workflows/df12-build-odw.jsfrom source viamake workflow-build.Follows the CodeRabbit implementation plan (guard at the top of
runTask,Option 2). Deviation: the CodeRabbit result shape included
proposals: []; theplan text omitted only
worktree/plan/branch, which are omitted here.Review-feedback follow-up
Addresses the three review warnings on the initial fix:
DRY_RUNearly return emits a structured, parseablelog()at the decision boundary carrying all four fields: task id,lane(derived from
task.isAddendum),stage=pre-worktree, andreason=dry-run.The returned result object's shape (
status,stage,detail,proposals)is unchanged. The
logsink is injectable in the test harness, so theregression suite asserts the emitted fields for both the normal and addendum
lanes.
fast-checkproperty(
tests/df12-build-odw-dry-run.property.test.mjs, 100 iterations) sweepsarbitrary task ids (fully arbitrary strings, roadmap-shaped dotted ids, and
branch-colliding ids), both lanes, and pooled repo states, asserting a dry
run always returns
status=dry-run/stage=pre-worktree, dispatches noagent, and leaves the durable git-state snapshot byte-for-byte unchanged. The
runTaskharness is extracted intotests/fixtures/load-run-task.mjsandshared with the regression suite.
docs/developers-guide.md(ODW workflowcontract) documents the pre-worktree boundary, records that the earlier
addendum-specific and post-design dry-run exits were removed as unreachable,
and cross-references the architecture
Enforcement boundarytable and theusers' guide
Workflow argumentssection so all three docs agree. No trackedroadmap/execplan work-item exists for Make df12-build-odw dryRun avoid real worktree creation #31, so none required a status update.
Gates pass:
make check-fmt,make lint,make typecheck,make test,make markdownlint, andmake nixieare all green.Closes #31
References