feat: structured conflict resolution with hold mechanism#39
Conversation
Add CLI hold/unhold commands that structurally block a quest's tools via the gate-guard hook when held. Gandalf uses this to pause quests during file conflict resolution detected by Palantir. Closes #14 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds a persistent hold/unhold mechanism: CLI commands, persisted state fields with file-locking, Herald events, gate-guard short‑circuit that blocks modification tools while held, unit tests for guard behavior, and documentation describing a Conflict Resolution Protocol. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as "Fellowship CLI"
participant State as "State File"
participant Herald as "Herald Log"
participant Hook as "Gate Guard Hook"
participant Tool as "Edit/Write/Bash Tool"
User->>CLI: fellowship hold --dir <worktree> --reason "conflict"
CLI->>State: WithLock: load → set Held=true, HeldReason="conflict" → save
CLI->>Herald: emit QuestHeld
CLI-->>User: print held confirmation
User->>Tool: attempt modification
Tool->>Hook: invoke GateGuard
Hook->>State: read Held/HeldReason
State-->>Hook: return Held=true, reason
Hook-->>Tool: block (exit 2) with message
Tool-->>User: operation blocked
User->>CLI: fellowship unhold --dir <worktree>
CLI->>State: WithLock: load → clear Held/HeldReason → save
CLI->>Herald: emit QuestUnheld
CLI-->>User: print unhold confirmation
User->>Tool: retry modification
Tool->>Hook: invoke GateGuard
Hook->>State: read Held
State-->>Hook: return Held=false
Hook-->>Tool: allow operation
Tool-->>User: success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/cmd/fellowship/main.go`:
- Around line 417-433: The hold/unhold code is doing an unsafe read-modify-write
on quest-state.json using state.Load → mutate s.Held/s.HeldReason → state.Save
which can race with other writers; change these paths to use the shared
atomic/locking helper in the state package (instead of open-coding Load/Save) so
updates are serialized — e.g., call the state package’s atomic update function
(e.g., state.Update/Mutate/WithLock or implement one) and perform the
Held/HeldReason mutation inside that callback, returning any save error from the
helper; apply the same change to the other block (lines corresponding to the
unhold path) so both hold and unhold use the lock/atomic update helper rather
than direct state.Load/state.Save.
In `@plugin/skills/fellowship/SKILL.md`:
- Around line 385-391: Update the hold semantics paragraph to explicitly state
that NotebookEdit is blocked by the gate-guard hook: when a quest is held (via
the fellowship hold command) the guard prevents Edit/Write/Bash/Agent/Skill
tools and also NotebookEdit from running; modify the paused-tools list (the
sentence referencing paused tools) to include the NotebookEdit symbol so the
documentation accurately reflects the guard behavior and that the quest cannot
resume notebook edits until unheld.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cfd0f45d-00a9-4122-9243-d76e6344e276
📒 Files selected for processing (7)
cli/cmd/fellowship/main.gocli/internal/herald/herald.gocli/internal/hooks/guard.gocli/internal/hooks/guard_test.gocli/internal/state/state.goplugin/skills/fellowship/SKILL.mdplugin/skills/quest/SKILL.md
| Hold the later quest immediately to prevent further divergence: | ||
|
|
||
| ```bash | ||
| fellowship hold --dir <later-quest-worktree> --reason "file conflict with <other-quest>: <file_path>" | ||
| ``` | ||
|
|
||
| This structurally blocks the held quest's Edit/Write/Bash/Agent/Skill tools via the gate-guard hook. The quest cannot proceed until unheld. |
There was a problem hiding this comment.
Document NotebookEdit as blocked during a hold.
Line 391 lists the paused tools, but the current guard behavior also blocks notebook edits while held. This section currently understates the hold semantics and can mislead teammates about what remains allowed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugin/skills/fellowship/SKILL.md` around lines 385 - 391, Update the hold
semantics paragraph to explicitly state that NotebookEdit is blocked by the
gate-guard hook: when a quest is held (via the fellowship hold command) the
guard prevents Edit/Write/Bash/Agent/Skill tools and also NotebookEdit from
running; modify the paused-tools list (the sentence referencing paused tools) to
include the NotebookEdit symbol so the documentation accurately reflects the
guard behavior and that the quest cannot resume notebook edits until unheld.
Extract spawn prompts, lead behavior, progress tracking, and conflict resolution into resources/ for progressive disclosure. Reduces SKILL.md from 613 to 197 lines — loaded into Gandalf's context at startup, resources loaded on-demand only when needed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ef9dda9b-392b-46c3-b832-862d09a32417
📒 Files selected for processing (5)
plugin/skills/fellowship/SKILL.mdplugin/skills/fellowship/resources/conflict-resolution.mdplugin/skills/fellowship/resources/lead-behavior.mdplugin/skills/fellowship/resources/progress-tracking.mdplugin/skills/fellowship/resources/spawn-prompts.md
✅ Files skipped from review due to trivial changes (1)
- plugin/skills/fellowship/resources/conflict-resolution.md
| When the user asks for "status" or Gandalf proactively reports progress: | ||
|
|
||
| ``` | ||
| ## Fellowship Status | ||
|
|
||
| | Task | Type | Phase | Progress | | ||
| |------|------|-------|----------| | ||
| | quest-auth-bug | Quest | Implement | ████░░ 3/5 | | ||
| | quest-rate-limit | Quest | Research | █░░░░░ 1/5 | | ||
| | scout-auth-analysis | Scout | Validating | ██░░ 2/3 | | ||
|
|
||
| **Quests:** 2 active | **Scouts:** 1 active | **Completed:** 0 |
There was a problem hiding this comment.
Surface held quests in the status format.
This report shape has no way to show that a quest is paused by hold, so a held quest will still read like a normal active quest. Please add a visible held indicator here (for example in Phase, a separate Status column, or alongside the progress cell) and include the hold reason when present.
Add state.WithLock for atomic load→mutate→save on quest-state.json, matching the WithStateLock pattern used for fellowship-state.json. Migrate gate approve, gate reject, hold, and unhold to use it. Also add NotebookEdit to the list of blocked tools in conflict resolution docs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d84b87ce-d829-4f98-a67d-6230c82c0c81
📒 Files selected for processing (3)
cli/cmd/fellowship/main.gocli/internal/state/state.goplugin/skills/fellowship/resources/conflict-resolution.md
- Add file locking to hook mutations (gate-submit, gate-prereq, metadata-track) via state.WithLock with ErrNoSave sentinel - Add hold/unhold as proactive commands in lead-behavior docs - Surface held quests in progress tracking status format - Explain hold mechanism in quest runner spawn prompt - Name concrete hold/unhold commands in SKILL.md conflict section - Fix usage text: list specific blocked tools instead of "all tools" Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Extract file locking into internal/filelock package with build tags: unix uses syscall.Flock, windows uses LockFileEx/UnlockFileEx via kernel32.dll. Fixes compilation on Windows. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (3)
cli/internal/state/state.go (2)
106-111: Useerrors.Isfor sentinel error comparison.Direct equality (
err == ErrNoSave) can fail if the error is wrapped. Useerrors.Is(err, ErrNoSave)for robust sentinel detection.♻️ Proposed fix
+import "errors"if err := fn(s); err != nil { - if err == ErrNoSave { + if errors.Is(err, ErrNoSave) { return nil } return err }
96-99: Silently ignoring unlock errors may hide issues.The deferred
syscall.Flock(..., LOCK_UN)discards its error. While unlock failures are rare, logging or returning them could help diagnose file system issues.plugin/skills/fellowship/SKILL.md (1)
145-149: Consider documenting blocked tools inline.The hold/unhold commands are now explicitly named (addressing prior feedback), but the blocked tools (Edit/Write/Bash/Agent/Skill/NotebookEdit) are only documented in spawn-prompts.md. Consider adding a brief note here that the hold mechanism blocks the same tools as gate-pending, or list them explicitly for completeness.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a593d91b-6512-4c94-9418-b3ad5adda979
📒 Files selected for processing (6)
cli/cmd/fellowship/main.gocli/internal/state/state.goplugin/skills/fellowship/SKILL.mdplugin/skills/fellowship/resources/lead-behavior.mdplugin/skills/fellowship/resources/progress-tracking.mdplugin/skills/fellowship/resources/spawn-prompts.md
✅ Files skipped from review due to trivial changes (1)
- plugin/skills/fellowship/resources/progress-tracking.md
🚧 Files skipped from review as they are similar to previous changes (1)
- plugin/skills/fellowship/resources/lead-behavior.md
justinjdev
deleted the
worktree-fellowship/structured-conflict-resolution
branch
1 participant
Summary
fellowship hold --dir <worktree> [--reason "msg"]andfellowship unhold --dir <worktree>CLI commands that structurally block a quest's tools via the gate-guard hookGateGuardto check held state before gate_pending — blocks Edit/Write/Bash/Agent/Skill/NotebookEdit whenheld: truequest_heldandquest_unheldherald tiding types for activity trackingTest plan
guard_test.go(blocks when held, includes reason, held priority over gate_pending)go test ./...)fellowship holdandfellowship unholdshow correct usageCloses #14
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests