Repository navigation
feat: port fellowship off the removed agent-teams API - #108
Conversation
Claude Code no longer exposes the agent-teams task tools, so the two hooks that keyed on TaskUpdate (metadata-track, completion-guard) had nothing to fire on. Their enforcement moves into the CLI: - `fellowship phase confirm --dir <worktree> --phase <phase>` records the phase prerequisite under the same validation (a valid phase equal to the quest's current phase) and never moves the phase. - `fellowship complete --dir <worktree>` ends the quest only in Review with no gate pending and no hold; gate-guard refuses the Bash form under the same rule (hooks.CompletionCheck), scanning the whole command line. Identity under the implicit team: a background agent spawned with the Agent tool runs in-process and shares the lead's session id, so a hook payload is the lead only when its session id matches AND it carries no agent_id. gate-guard's lead exemption and worktree-guard's rules use that, `init` no longer records the lead's own id against a quest, and `init --phase` on an existing quest also requires the process to stand in the main working tree. gate-submit reads the SendMessage body from `message` (with `content` as a fallback) and returns the enrichment under the same field. The --task-id flags and the TaskID/TeamName fields are gone; the task_id/team_name columns stay in the schema, unused, so no migration is needed. Also: unknown gates.autoApprove entries are ignored with a warning instead of failing init, and the out-of-date-store block message says the upgrade command must be run on its own. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XpCG85waNcdZjvMQE82QfT
Teammates are named background agents: spawned with Agent(name: ...), addressed with SendMessage(to: <name>) (which resumes an idle teammate with its context intact), and stopped with TaskStop. Quest identity is the quest name; the store is the only coordination state, and the lead registers each worktree there before spawning. The gate flow keeps its shape — /lembas, `fellowship phase confirm`, one [GATE] message to main, end the turn — and completion is `fellowship complete` followed by a [COMPLETE] envelope with the PR URL. Scouts report with [REPORT]; palantir drops the task tools; the shutdown handshake is gone. hooks.json loses its two TaskUpdate matchers. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XpCG85waNcdZjvMQE82QfT
… identity Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XpCG85waNcdZjvMQE82QfT
📝 WalkthroughWalkthroughThe change moves fellowship coordination from task metadata and team hooks to store-backed quest state, explicit lifecycle commands, named background agents, and subagent-aware hook enforcement. ChangesImplicit-team fellowship lifecycle
Estimated code review effort: 5 (Critical) | ~120 minutes Merge Risk: 🟠 High · up to The implicit-team workflow can execute crafted note content, apply lifecycle enforcement to the wrong quest, or complete another quest. These material issues should be fixed before merge. Sequence Diagram(s)sequenceDiagram
participant Lead
participant Agent
participant FellowshipCLI
participant Hooks
participant Store
Lead->>FellowshipCLI: register quest and worktree
FellowshipCLI->>Store: save quest and worktree
Lead->>Agent: spawn named background agent
Agent->>Hooks: run agent-track after init
Hooks->>Store: map agent to quest
Agent->>FellowshipCLI: phase confirm --dir
FellowshipCLI->>Hooks: validate current phase
Agent->>Hooks: submit gate message
Hooks->>Store: record gate state
Lead->>FellowshipCLI: approve gate
FellowshipCLI->>Agent: send approval message
Agent->>FellowshipCLI: complete --dir
FellowshipCLI->>Hooks: validate Review and gate state
Hooks-->>FellowshipCLI: allow completion
FellowshipCLI->>Store: mark quest completed
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 66.02% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 103 functions across 33 files. (13 skipped: 13 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugin/agents/palantir.md (1)
126-127: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winInjection (CWE-78): Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection')
Exploitability: Moderate
Do not interpolate note content into a Bash command.
The Palantir agent inserts discovery text into a double-quoted
events postcommand. Shell metacharacters can execute commands or alter arguments before the CLI receives the text. Add an argv-safe or non-shell input path and remove the claim that there are “no quoting hazards.”
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Team
Run ID: 998f8f3d-a9bb-46b6-90fd-864d625331f6
📒 Files selected for processing (51)
CONTRIBUTING.mdREADME.mdcli/cmd/fellowship/erase_test.gocli/cmd/fellowship/gate.gocli/cmd/fellowship/hook.gocli/cmd/fellowship/hook_test.gocli/cmd/fellowship/implicit_team_test.gocli/cmd/fellowship/main.gocli/cmd/fellowship/phase.gocli/cmd/fellowship/smoke_subprocess_test.gocli/cmd/fellowship/state.gocli/internal/dashboard/server_test.gocli/internal/events/events.gocli/internal/fellowship/fellowship.gocli/internal/fellowship/fellowship_test.gocli/internal/health/health.gocli/internal/health/health_test.gocli/internal/hooks/complete_test.gocli/internal/hooks/completion.gocli/internal/hooks/completion_test.gocli/internal/hooks/guard.gocli/internal/hooks/input.gocli/internal/hooks/input_test.gocli/internal/hooks/isolation.gocli/internal/hooks/isolation_test.gocli/internal/hooks/leadonly.gocli/internal/hooks/metadata.gocli/internal/hooks/metadata_test.gocli/internal/hooks/phase.gocli/internal/hooks/phase_test.gocli/internal/hooks/submit.gocli/internal/hooks/submit_test.gocli/internal/state/state.goplugin/CLAUDE.mdplugin/agents/_protocol.mdplugin/agents/balrog.mdplugin/agents/palantir.mdplugin/agents/scout.mdplugin/commands/rekindle.mdplugin/hooks/hooks.jsonplugin/hooks/scripts/fellowship.shplugin/skills/fellowship/SKILL.mdplugin/skills/fellowship/resources/isolation.mdplugin/skills/fellowship/resources/lead-behavior.mdplugin/skills/fellowship/resources/progress-tracking.mdplugin/skills/fellowship/resources/spawn-prompts.mdplugin/skills/quest/SKILL.mdplugin/skills/scout/SKILL.mdsite/src/routes/changelog/+page.sveltesite/src/routes/concepts/+page.sveltesite/src/routes/how-it-works/+page.svelte
💤 Files with no reviewable changes (5)
- plugin/hooks/hooks.json
- cli/internal/hooks/metadata_test.go
- cli/internal/hooks/metadata.go
- cli/internal/dashboard/server_test.go
- cli/internal/health/health_test.go
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
A background agent never stands in its worktree: its working directory is the lead's, the main repo root, and a bare `cd` does not persist between its Bash calls. The hooks keyed every quest lookup off that cwd, so for such a teammate gate-guard's lead-only refusal and gate-submit never ran. Hooks now resolve a subagent's quest from what the call names — the --dir of a fellowship command, the file an Edit/Write targets — and remember it in agent_quests (schema migration 6) keyed by the payload's agent_id, so SendMessage and Skill calls resolve too. A PostToolUse Bash hook, agent-track, records the mapping after the teammate's `fellowship init --dir <worktree>`. The bare-cd refusal applies only to the lead's own conversation. Also from review: `fellowship complete --dir <X>` is judged against the quest --dir names; gate-submit's enrichment returns the whole tool_input with only the body rewritten, so `to` survives a harness that replaces rather than merges; and `fellowship init` records a session id against a quest only when a lead is recorded and the id differs from it, so the lead's own id can never be written there before the lead is known. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XpCG85waNcdZjvMQE82QfT
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XpCG85waNcdZjvMQE82QfT
An in-process teammate keeps the main repo root as its working directory
and a bare `cd` does not persist, so every teammate-facing text now says:
every fellowship command takes --dir {worktree_path}, every path is
absolute inside the worktree, the isolation self-check runs `git -C`, and
the checkpoint lives in the worktree's data directory. The spawn prompt
carries {worktree_path}; lead-behavior points at the history and the
quest_completed event as the evidence a quest ran `fellowship complete`.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XpCG85waNcdZjvMQE82QfT
- The lead check behind a phase move on an existing quest fails closed: a working directory that cannot be resolved to a git tree is not the main worktree, so a caller in a non-git directory cannot slip past it. - `events post --detail -` reads the detail from stdin, and palantir's alert-logging examples use a single-quoted heredoc, so text written by other agents never lands on a shell command line. - `complete`'s usage and CONTRIBUTING name the no-hold condition. - Balrog and palantir message examples carry an envelope header line. - /fellowship and the scout-promotion steps register a quest, with its worktree, before spawning it; a quest that had to create its own worktree passes --quest to init so its state is not created under the directory's name. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XpCG85waNcdZjvMQE82QfT
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
plugin/skills/fellowship/SKILL.md (1)
126-126: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDo not re-register an existing quest with
state add-quest.
state add-questupserts by quest name and defaults the stored status toactive. If this step runs for a completed or cancelled quest, it can reactivate that quest. The pre-flight already registers the worktree, so remove this call or usestate update-quest --worktree.cli/internal/hooks/guard.go (1)
70-70: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAuthorization Bypass (CWE-863): Incorrect Authorization
Reachability: External · Exploitability: Moderate
Reject hook payloads with missing or empty
agent_id.
ParseInputaccepts payloads withoutagent_id, andIsLeadPayloadthen treats an empty value as the lead whensession_idmatchesLeadSessionID. A subagent can therefore reach lead-only checks without an agent identity. Fail closed beforeGateGuardevaluates the payload.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Team
Run ID: cdc76b8f-2cf0-442a-8a2d-1107536c8dfe
📒 Files selected for processing (31)
CONTRIBUTING.mdREADME.mdcli/cmd/fellowship/claimlead_attack_test.gocli/cmd/fellowship/events.gocli/cmd/fellowship/gate.gocli/cmd/fellowship/hook.gocli/cmd/fellowship/implicit_team_test.gocli/cmd/fellowship/main.gocli/cmd/fellowship/smoke_subprocess_test.gocli/cmd/fellowship/state.gocli/internal/db/migrations.gocli/internal/db/schema.gocli/internal/hooks/complete_test.gocli/internal/hooks/guard.gocli/internal/hooks/input.gocli/internal/hooks/input_test.gocli/internal/hooks/leadonly.gocli/internal/hooks/submit.gocli/internal/state/agent.goplugin/agents/balrog.mdplugin/agents/palantir.mdplugin/commands/rekindle.mdplugin/hooks/hooks.jsonplugin/hooks/scripts/fellowship.shplugin/skills/fellowship/SKILL.mdplugin/skills/fellowship/resources/isolation.mdplugin/skills/fellowship/resources/lead-behavior.mdplugin/skills/fellowship/resources/spawn-prompts.mdplugin/skills/lembas/SKILL.mdplugin/skills/quest/SKILL.mdsite/src/routes/changelog/+page.svelte
🚧 Files skipped from review as they are similar to previous changes (10)
- cli/internal/hooks/input_test.go
- cli/internal/hooks/input.go
- plugin/agents/balrog.md
- site/src/routes/changelog/+page.svelte
- plugin/hooks/scripts/fellowship.sh
- README.md
- plugin/skills/fellowship/resources/isolation.md
- plugin/commands/rekindle.md
- plugin/skills/fellowship/resources/lead-behavior.md
- cli/cmd/fellowship/main.go
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
| // alert). Reading it from stdin lets the caller pass it without ever | ||
| // putting it on a shell command line, where its metacharacters would run. | ||
| if *detail == "-" { | ||
| data, err := io.ReadAll(os.Stdin) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect existing event-size limits and stdin readers before selecting the limit.
rg -n -C 3 'LimitReader|ReadAll\(os\.Stdin\)|max.*(detail|event)|Detail.*len' cliRepository: justinjdev/fellowship
Length of output: 159
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- cli/cmd/fellowship/events.go ---'
sed -n '1,220p' cli/cmd/fellowship/events.go
printf '%s\n' '--- relevant symbols and callers ---'
rg -n -C 4 --glob '*.go' 'detail|Detail|ReadAll|event' cli/cmd/fellowshipRepository: justinjdev/fellowship
Length of output: 32372
🤖 get_repo_knowledge executed:
get_repo_knowledge justinjdev/fellowship /tmp/coderabbit-repo-knowledge/justinjdev-fellowship-08bfdc09/architecture /tmp/coderabbit-repo-knowledge/justinjdev-fellowship-08bfdc09/learnings /tmp/coderabbit-repo-knowledge/justinjdev-fellowship-08bfdc09/conventions
Length of output: 23843
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- event implementation files ---'
fd -t f . cli/internal/events cli/internal/db | sort
printf '%s\n' '--- event storage and detail constraints ---'
rg -n -C 5 'type Event|Detail|CREATE TABLE.*event|events \(|event_detail|detail.*(limit|length)|length.*detail|Max.*Detail' cli/internal/events cli/internal/db README.md CONTRIBUTING.md 2>/dev/null || true
printf '%s\n' '--- documented events post contract ---'
rg -n -C 4 'events post|event detail|detail text|palantir_notes' --glob '*.md' --glob '*.go' --glob '*.json' .Repository: justinjdev/fellowship
Length of output: 24198
Bound --detail - input before buffering it.
runEventsPost calls io.ReadAll(os.Stdin) without a maximum. A large piped detail can exhaust process memory before validation or persistence. Define an explicit detail limit, read at most that limit plus one byte, and reject oversized input.
| if parseErr != nil && name != "gate-guard" { | ||
| fmt.Fprintln(os.Stderr, "fellowship: malformed hook input — blocking for safety") | ||
| return 2 | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🤖 get_repo_knowledge executed:
get_repo_knowledge justinjdev/fellowship /tmp/coderabbit-repo-knowledge/justinjdev-fellowship-08bfdc09/architecture /tmp/coderabbit-repo-knowledge/justinjdev-fellowship-08bfdc09/learnings
Length of output: 22996
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- hook.go relevant definitions ---'
rg -n -C 18 'ParseInput|no quest|quest|parseErr|unregisteredQuestWorktree|GateSubmit|func .*Hook|hook' cli/cmd/fellowship/hook.go | head -n 260
printf '%s\n' '--- ParseInput and worktree helpers ---'
rg -n -C 16 'func ParseInput|func .*Quest|unregisteredQuestWorktree|MainRepoRoot|CanonicalPath|GateSubmit' cli/internal cli/cmd/fellowship -g '*.go' | head -n 320Repository: justinjdev/fellowship
Length of output: 31348
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- hook.go lines 100-175 ---'
sed -n '100,175p' cli/cmd/fellowship/hook.go
printf '%s\n' '--- ParseInput implementation ---'
rg -n -A 45 -B 8 '^func ParseInput' cli/internal/hooks -g '*.go'
printf '%s\n' '--- direct callers and tests ---'
rg -n -C 10 'ParseInput\(|malformed hook input|gate-submit|main worktree|no quest|unregisteredQuestWorktree' cli/cmd/fellowship cli/internal -g '*_test.go' -g '*.go' | head -n 360Repository: justinjdev/fellowship
Length of output: 28388
🏁 Script executed:
#!/bin/bash
set -eu
sed -n '100,175p' cli/cmd/fellowship/hook.go
rg -n -A 45 -B 8 '^func ParseInput' cli/internal/hooks -g '*.go'
rg -n -C 10 'ParseInput\(|malformed hook input|unregisteredQuestWorktree|GateSubmit' cli/cmd/fellowship cli/internal -g '*.go' | head -n 360Repository: justinjdev/fellowship
Length of output: 30097
Authorization Bypass (CWE-863): Incorrect Authorization
Reachability: External
Block malformed non-guard hooks before quest lookup.
For gate-submit from the main worktree, malformed input reaches the no-quest success return before this check runs. Move the check directly after hooks.ParseInput and add a regression test that asserts exit code 2.
Proposed fix
input, parseErr := hooks.ParseInput(stdin)
if parseErr != nil {
input = &hooks.HookInput{}
}
+if parseErr != nil && name != "gate-guard" {
+ fmt.Fprintln(os.Stderr, "fellowship: malformed hook input — blocking for safety")
+ return 2
+}
// Find the quest for this tool call.
...
-if parseErr != nil && name != "gate-guard" {
- fmt.Fprintln(os.Stderr, "fellowship: malformed hook input — blocking for safety")
- return 2
-}| if strings.Contains(out, `"deny"`) { | ||
| t.Fatalf("%s: gate was denied with prerequisites met: %s", phase, out) | ||
| } | ||
| if out != "" && (!strings.Contains(out, `"message":"[GATE] `+phase) || !strings.Contains(out, `"to":"main"`)) { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Require enriched hook output.
The out != "" guard accepts an empty hook response. A regression that drops updatedInput then passes this smoke test. Assert both required fields unconditionally.
Proposed fix
- if out != "" && (!strings.Contains(out, `"message":"[GATE] `+phase) || !strings.Contains(out, `"to":"main"`)) {
+ if !strings.Contains(out, `"message":"[GATE] `+phase) || !strings.Contains(out, `"to":"main"`) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if out != "" && (!strings.Contains(out, `"message":"[GATE] `+phase) || !strings.Contains(out, `"to":"main"`)) { | |
| if !strings.Contains(out, `"message":"[GATE] `+phase) || !strings.Contains(out, `"to":"main"`) { |
| if dir != "" && p.StateForDir != nil { | ||
| resolved, err := p.StateForDir(dir) | ||
| if err != nil { | ||
| return HookResult{Block: true, Message: fmt.Sprintf("fellowship: cannot resolve the quest for --dir %q: %v — blocking for safety.", dir, err)} | ||
| } | ||
| if resolved == nil { | ||
| return HookResult{Block: true, Message: fmt.Sprintf("fellowship: no quest is registered for --dir %q — nothing to complete.", dir)} | ||
| } | ||
| target = resolved |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
sed -n '1,260p' cli/cmd/fellowship/complete.go
printf '\n--- hook identity and command wiring ---\n'
sed -n '1,230p' cli/cmd/fellowship/hook.go
printf '\n--- state ownership helpers ---\n'
rg -n "FindQuestByAgent|agent_quests|complete|QuestName|StateForDir" cli/internal cli/cmd/fellowship -g '*.go' | head -160Repository: justinjdev/fellowship
Length of output: 27166
🏁 Script executed:
sed -n '1,260p' cli/cmd/fellowship/complete.go
printf '\n--- hook identity and command wiring ---\n'
sed -n '1,230p' cli/cmd/fellowship/hook.go
printf '\n--- bounded ownership and completion references ---\n'
rg -n "FindQuestByAgent|agent_quests|complete|QuestName|StateForDir" cli/internal cli/cmd/fellowship -g '*.go' | head -160Repository: justinjdev/fellowship
Length of output: 26154
🏁 Script executed:
sed -n '70,155p' cli/cmd/fellowship/phase.go
printf '\n--- subagent quest resolution ---\n'
sed -n '600,665p' cli/cmd/fellowship/hook.go
printf '\n--- completion integration tests ---\n'
sed -n '110,190p' cli/cmd/fellowship/implicit_team_test.goRepository: justinjdev/fellowship
Length of output: 8200
IDOR (CWE-639): Authorization Bypass Through User-Controlled Key (IDOR)
Reachability: External · Exploitability: Moderate
Bind --dir completion to the requesting quest.
runComplete resolves and completes any quest named by --dir. Neither it nor GateGuard checks agent ownership. A subagent can therefore complete another quest while it is in Review. Add coverage for agent A targeting quest B, and reject the operation unless agent A is authorized for quest B.
| if !isFellowshipBinary(tokens[i]) { | ||
| continue | ||
| } | ||
| if d := dirArgFrom(tokens[i+1:]); d != "" { | ||
| dirs = append(dirs, d) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Authorization Bypass (CWE-863): Incorrect Authorization
Reachability: External · Exploitability: Moderate
Restrict FellowshipDirArgs to executed command positions.
FellowshipDirArgs accepts fellowship from ordinary arguments. For example, echo fellowship status --dir /other; touch <current-quest-file> can select /other. resolveSubagentQuest uses that path before FindQuestByAgent, which can apply another quest’s gate policy. Track command positions while preserving supported wrappers such as sh -c, and add a regression test for this case.
|
|
||
| Two CLI commands carry enforcement that used to live in hooks: `fellowship phase confirm --dir <worktree> --phase <phase>` records the phase prerequisite only when the phase is valid and equals the quest's current phase, and never changes the phase; `fellowship complete --dir <worktree>` is allowed only in Review with no gate pending and no hold, marks the history completed and sets the store status to `completed` — gate-guard also refuses the Bash form of `fellowship complete` when that check fails, so the enforcement is structural, not prompt-only. | ||
|
|
||
| **Resolving a subagent's quest.** A session standing in its own worktree resolves through its cwd, same as always. An in-process teammate never does: it keeps the lead's working directory for its whole life, and a bare `cd` does not persist between its Bash calls, so hooks instead resolve its quest from what the tool call names — the `--dir` a `fellowship` command gives, or the file its Edit/Write targets — and remember that mapping in `agent_quests` (schema migration 6), keyed by the payload's `agent_id`, so calls that name nothing (a `SendMessage` for `[GATE]`, a `Skill` for `/lembas`) still resolve. `agent-track`, a PostToolUse hook on `Bash`, records the mapping after the teammate's first `fellowship init --dir <worktree>`. `fellowship complete --dir <X>` is judged by gate-guard against the quest the `--dir` names, not the caller's cwd quest, and gate-submit's enrichment returns the whole `tool_input` with only `message` rewritten, so `to`/`summary` survive. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🤖 get_repo_knowledge executed:
get_repo_knowledge justinjdev/fellowship /tmp/coderabbit-repo-knowledge/justinjdev-fellowship-08bfdc09/architecture
Length of output: 20270
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate files ---'
git ls-files | rg '(^|/)(agent-track|gate-guard|gate-submit|.*quest.*|.*hook.*|.*fellowship.*)$|(^|/)CONTRIBUTING\.md$'
printf '%s\n' '--- relevant symbols and references ---'
rg -n --glob '!CONTRIBUTING.md' 'agent-track|agent_quests|gate-guard|gate-submit|agent_id|fellowship init|PostToolUse' .Repository: justinjdev/fellowship
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- cli/internal/state/agent.go ---'
cat -n cli/internal/state/agent.go
printf '%s\n' '--- agent quest schema and lookup contract ---'
sed -n '35,75p' cli/internal/db/schema.go
sed -n '120,150p' cli/internal/db/migrations.go
printf '%s\n' '--- hook resolution and agent-track paths ---'
sed -n '70,115p' cli/cmd/fellowship/hook.go
sed -n '225,275p' cli/cmd/fellowship/hook.go
sed -n '600,680p' cli/cmd/fellowship/hook.go
printf '%s\n' '--- implicit-team mapping and pathless gate tests ---'
sed -n '330,425p' cli/cmd/fellowship/implicit_team_test.goRepository: justinjdev/fellowship
Length of output: 16723
Authorization Bypass (CWE-639): Authorization Bypass Through User-Controlled Key (IDOR)
Reachability: Internal · Exploitability: Moderate
Bind agent-track to the agent's original quest.
resolveSubagentQuest accepts any quest resolved from a named --dir or target path, and agent-track upserts that quest for the agent. Agent A can therefore name quest B, overwrite its mapping, and direct a later path-less [GATE] or /lembas operation to quest B. Reject cross-quest paths or prevent remapping after the first binding. Add a regression test for this sequence.
| After sending each alert via `SendMessage`, record it in the fellowship event log so `/retro` can analyze it later. The CLI writes the entry for you — no `jq`, no log file to append to. The detail text comes from other quests' notes, so never paste it into the command line: `--detail -` reads it from stdin, and a single-quoted heredoc keeps the shell from expanding anything in it: | ||
|
|
||
| ```bash | ||
| ~/.claude/fellowship/bin/fellowship events post --quest "<quest_name>" --type "<type>" --detail "<alert message>" | ||
| ~/.claude/fellowship/bin/fellowship events post --quest "<quest_name>" --type "<type>" --detail - <<'EOF' | ||
| <alert message> | ||
| EOF |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- plugin/agents/palantir.md:100-140 ---'
sed -n '100,140p' plugin/agents/palantir.md
printf '%s\n' '--- nearby references to event logging and heredocs ---'
rg -n -C 3 'events post|<<.?EOF|alert message|discovery' plugin/agents/palantir.mdRepository: justinjdev/fellowship
Length of output: 7017
🤖 get_repo_knowledge executed:
get_repo_knowledge justinjdev/fellowship /tmp/coderabbit-repo-knowledge/justinjdev-fellowship-08bfdc09/architecture /tmp/coderabbit-repo-knowledge/justinjdev-fellowship-08bfdc09/conventions
Length of output: 19097
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- Palantir frontmatter and note workflow ---'
sed -n '1,95p' plugin/agents/palantir.md
printf '%s\n' '--- note command definitions and callers ---'
rg -n -C 3 'notes (add|post|list)|notes add|Notes entry|discovery' plugin cli --glob '*.md' --glob '*.go' --glob '*.json' | head -220Repository: justinjdev/fellowship
Length of output: 18752
🌐 Web query:
Bash quoted heredoc delimiter line exactly EOF behavior
💡 Result:
In Bash, quoting the heredoc delimiter (e.g., <<'EOF' or <<"EOF") causes the shell to treat the heredoc body as a literal string, disabling all variable expansion, command substitution, and arithmetic expansion [1][2][3][4]. Behavior of Quoted Delimiters: When any part of the heredoc delimiter word is quoted, the shell performs quote removal on the word to determine the final delimiter string [1][3]. Crucially, the entire heredoc body is then processed without any expansions [1][2]. This is the standard method for preventing Bash from interpreting special characters like $, , \, and! within the block [2]. Behavior of Unquoted Delimiters: If the delimiter is unquoted (e.g., <<EOF), Bash performs parameter expansion, command substitution, and arithmetic expansion on the content of the heredoc before passing it to the command [1][3]. In this mode, characters such as $ and retain their special meaning, and the backslash character () must be used to escape them if literal output is required [1]. Key Requirements for the Closing Delimiter: Regardless of whether the delimiter is quoted, the closing delimiter line must adhere to strict requirements: 1. It must appear on its own line [1]. 2. It must be identical to the starting delimiter (after quote removal) [1][3]. 3. It must typically start at the first character position of the line (no leading whitespace), unless the <<- operator is used, which allows for indented tabs [1][2]. 4. It must contain no trailing characters, including spaces, as these will prevent the shell from recognizing the closing delimiter, causing the script to hang or fail [2].
Citations:
- 1: https://stackoverflow.com/questions/2500436/how-does-cat-eof-work-in-bash
- 2: https://tldp.org/LDP/abs/html/here-docs.html
- 3: https://superuser.com/questions/1436906/need-to-expand-a-variable-in-a-heredoc-that-is-in-quotes
- 4: https://unix.stackexchange.com/questions/793230/is-it-possible-to-define-a-bash-heredoc-with-a-multi-word-delimiter-that-expands
Injection (CWE-78): Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection')
Reachability: Internal · Exploitability: Moderate
Do not use a fixed EOF delimiter for note text.
If <alert message> or <discovery> contains a line exactly EOF, the heredoc closes early. Following note lines can then execute as shell commands when Palantir runs the generated Bash. Quoting EOF blocks expansion but does not prevent delimiter collisions. Use a shell-safe stdin transport or generate and validate a delimiter absent from the complete note.
2 participants
Claude Code 2.1.261 no longer exposes
TaskCreate,TaskUpdate,TaskGet,TaskList,TeamCreate, orTeamDelete, andSendMessagehas notypefield, so/fellowshipcould not run at all on current Claude Code. This ports the plugin to what actually exists: one implicit team per session,Agent(name: ...),SendMessage(to: <name>),ListAgents, andTaskStop. Every tool schema was loaded and read in a live session before designing against it, and the key behaviours were cross-checked against the official docs (hooks, sub-agents, agent-teams, env-vars pages on code.claude.com).What was removed
TaskCreate/TaskUpdate/TaskGet/TaskList/TeamCreate/TeamDeleteacrossplugin/(/fellowshipand its resources,/quest,/scout,/rekindle,scout.md,palantir.md).shutdown_request/shutdown_responsehandshake and the"team-lead"recipient (_protocol.md,balrog.md,palantir.md,scout.md, spawn prompts).TaskUpdatematchers inplugin/hooks/hooks.jsonand themetadata-track/completion-guardhook handlers in the CLI.--task-idonstate add-quest,state add-scout,state update-quest; the{task_id}and{team_name}spawn-prompt placeholders; theTaskID/TeamNamefields in the store types andstate.State. Thetask_id/team_namecolumns stay in the schema, unused — dropping them needs a migration and a rewrite of the changelog triggers for no gain.What replaced it
TeamCreate/TeamDeletestate init --name),{team_name}→{fellowship_name}.TaskCreate+ task idsAgent(name: "<quest_name>", subagent_type: "general-purpose", run_in_background: true);SendMessage(to: "<quest_name>")resumes it with context intact. Teammates reach the lead withSendMessage(to: "main").TaskUpdate(metadata.worktree_path)/TaskGetin gate approvalstate add-quest --worktree/state update-quest --worktreebefore spawning; the quest readsstate show --json; the lead approves withgate approve --dir <worktree>.TaskUpdate(metadata.phase)+metadata-trackhookfellowship phase confirm --dir <worktree> --phase <phase>. Same validation: a valid phase and equal to the quest's current phase, otherwise refused with the existing notice text. It never moves the phase. gate-guard permits it in every phase (only a pending gate or a hold block it, like any Bash).TaskUpdate(status: completed)+completion-guardhookfellowship complete --dir <worktree>. Allowed only in Review with no gate pending and no hold; marks history and store statuscompleted. gate-guard refuses the Bash form under the same rule (hooks.CompletionCheck), scanning through&&/;/$(...)/sh -c/eval, and judges--dir <X>against the quest X names. The teammate then sends[COMPLETE] <quest>+PR: <url>tomain, defined next to[GATE].shutdown_requestTaskStop(task_id: "<name>")(schema: "To stop a background agent spawned with a name, pass that name as task_id").[REPORT] <scout_name>envelope; progress shows Active until it arrives, then Done. Noupdate-scout --status— it would need a schema migration for a signal the message already carries.TaskGet/TaskListstate show --json(task_description, worktrees). Tools:SendMessage, Bash.TeamCreate/TaskCreatestate init --claim-lead,state add-quest --worktree,Agent(name: <quest_name>)with the Resume variant.state show --json,health --json,group show --json.Also fixed because the new tool's payload differs:
gate-submitreads the SendMessage body frommessage(withcontentas a fallback) and returns the wholetool_inputwith only that field rewritten, soto/summarysurvive a harness that replaces rather than merges. Without the first part no gate would ever have been detected.Lead identity under the implicit team (found during verification)
Probed live: a background agent spawned with
Agentruns in-process, its Bash environment carries the lead'sCLAUDE_CODE_SESSION_IDand no agent-identifying variable, and its hook payloads carry the samesession_idplusagent_id/agent_type(confirmed by the hooks reference: "the input carries the agent_id and agent_type … that identify the subagent"). Under the old code every teammate would therefore have passed as the lead. The CLI now treats:agent_id(hooks.IsLeadPayload); gate-guard's lead exemption and worktree-guard rule (1) use it, and worktree-guard rule (3) blocks any subagent payload writing source into the main tree;fellowship initrecords a session id against a quest only when a lead is recorded and the id differs from it (otherwise--claim-leadwould refuse the lead itself, and the lead's own id could be written there before the lead was known);init --phase/--plan-skipon an existing quest, server-side, as the lead only with the lead's session id and a process cwd known to be in the main working tree (an unresolvable cwd fails closed); the Bash form from a teammate is refused first by gate-guard, which sees theagent_id.After code review: the teammate never stands in its worktree
A
/code-review highpass on the branch found that the hooks keyed every quest lookup off the hook process's working directory, and a live probe then showed why that matters: inside a background agent a barecddoes not persist to the next Bash call, so an in-process teammate's cwd is the main repo root for its whole life. Under the first cut, gate-guard's lead-only refusal and gate-submit would never have run for such a teammate. Fixed in12e528dand the commits after it:--dirof anyfellowshipcommand, the file an Edit/Write targets — and remember it inagent_quests(schema migration 6) keyed by the payload'sagent_id, so SendMessage and Skill calls resolve too; a new PostToolUse Bash hookagent-trackrecords the mapping after the teammate'sfellowship init --dir <worktree>;cdrefusal applies only to the lead's own conversation;fellowship complete --dir <X>is judged against the quest--dirnames, not the caller's;tool_inputwith onlymessagerewritten;fellowship initrecords a session id against a quest only when a lead is recorded and the id differs;cd; every fellowship command takes--dir {worktree_path}; every path is absolute inside the worktree; the isolation self-check runsgit -C {worktree_path}; the checkpoint lives at{worktree_path}/.fellowship/checkpoint.md. The spawn prompt carries{worktree_path}.The smoke test now runs the teammate's hooks from the main root, which is where a real teammate stands. The review's fifth finding (lead-behavior claiming only
fellowship completesets the store status) is fixed in prose: the history status plus thequest_completedevent are the evidence.CodeRabbit's seven inline findings are addressed in
6912b41: the lead check behind a phase move fails closed on an unresolvable cwd;events post --detail -reads the detail from stdin and palantir's alert-logging examples use a single-quoted heredoc, so text written by other agents never lands on a shell command line; the no-hold condition oncompleteis documented; balrog and palantir message examples carry an envelope header; quests are registered with their worktree before spawning; a quest that had to create its own worktree passes--questtoinit.Enforcement properties re-verified
Every property in README "Gate enforcement" and "Closed four ways past the gates" is covered by a test on this branch:
TestImplicitTeamSmoke,TestGateGuard_*);[GATE](TestImplicitTeamSmoke,TestGateSubmit_ReadsMessageField);TestRunComplete,TestRunHookWith_GateGuardRefusesCompleteBeforeReview,TestGateGuard_RefusesCompleteOutsideReview,TestGateGuard_CompleteDirIsJudgedAgainstThatQuest);phase confirmcannot move a phase (TestRunPhaseConfirm,TestConfirmPhase);init --phasefrom a teammate refused, now also for an in-process teammate sharing the lead's session id, from its worktree, from the main root, or from a non-git directory (TestRunInit_PhaseMoveNeedsTheMainWorktreeToo,TestRunInit_PhaseMoveRefusedFromANonGitDirectory,TestRunHookWith_SubagentPayloadIsNotTheLead,TestRunHookWith_SubagentAtMainRootResolvesItsQuest);TestRunHookWith_StoreWriteIsRefusedEverywhere);TestIsolationGuard_Subagent*);--dir, target path, and the agent mapping, and enforced there (TestRunHookWith_SubagentAtMainRootResolvesItsQuest,TestRunInit_RecordsNoSessionWithoutALead);fellowship.shcomment and CONTRIBUTING updated).go test -race ./...,go vet,gofmt -land the site build are clean.Smoke test
The hook-mediated end-to-end run cannot be driven non-interactively here: the fellowship plugin is not installed in this session, so its hooks do not fire on my subagents' tool calls. What was verified instead:
Agent(name: "probe-quest")ended its turn;SendMessage(to: "probe-quest")resumed it and it answered from its own memory without tools — the resume-by-name mechanism the gate flow depends on works. The same probe reported its environment: the lead'sCLAUDE_CODE_SESSION_ID, no agent id. A second probe showed a barecddoes not persist between a subagent's Bash calls.TestImplicitTeamSmoke(cli/cmd/fellowship/smoke_subprocess_test.go) drives the real built binary in a throwaway repo with real hook payloads (the lead's session id + anagent_id, as Claude Code sends for a subagent), with every teammate hook run from the main repo root. Exact sequence and hook messages:Secondary findings from the live test
gates.autoApprovewith retired names (Onboard,Adversarial,Review):fellowship initnow keeps the valid entries and prints one warning naming the unknown ones and the file to fix, instead of exiting 1 (TestRunInit_IgnoresUnknownAutoApproveGatesWithAWarning).cd, with no&&or pipe. The matcher was deliberately left strict: it is the one allowance past a gate hook that cannot read the gate flag, and chaining is exactly what it must not accept.go build -ldflags "-X main.version=<version>" -o fellowship ./cmd/fellowshipand that a plain build reportsdev.No version bump;
docs/untouched.🤖 Generated with Claude Code