fix(agent-hooks): drop hook status whose cwd disproves its pane - #11094
kunsanglee wants to merge 20 commits into
Conversation
Agent CLIs increasingly pre-warm a shared background daemon, and that daemon keeps the Orca pane env of whichever pane happened to spawn it. Every session it later hosts inherits ORCA_PANE_KEY/ORCA_WORKTREE_ID from that first pane, so a session running in workspace A reports workspace B's pane and its prompt, state, and subagents render on B's sidebar card while A shows nothing. Cross-check the session cwd the agent reports in its own hook payload against the worktree path already encoded in the reported worktreeId, and refuse the event when the two are disjoint. Ambiguity keeps the old behavior: a missing cwd, a non-path worktree id, mixed WSL/UNC notation, or either path nested in the other all stay attributed, since dropping a real status row is the worse failure. The relay forwards cwd alongside the normalized payload so remote sessions get the same check.
|
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:
📝 WalkthroughWalkthroughAgent hook payloads now extract and preserve the agent-reported session CWD through normalization and relay forwarding. Shared path utilities compare CWDs with reported worktrees across supported path formats. The main hook server filters contradictory local and remote status events, records 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: cc09d14c-d4a3-4bb8-9ab5-5802c5ebcf85
📒 Files selected for processing (9)
src/main/agent-hooks/server-cwd-attribution.test.tssrc/main/agent-hooks/server.tssrc/relay/agent-hook-server.test.tssrc/relay/agent-hook-server.tssrc/shared/agent-hook-cwd-attribution.test.tssrc/shared/agent-hook-cwd-attribution.tssrc/shared/agent-hook-listener.tssrc/shared/agent-hook-relay.tssrc/shared/telemetry-events.ts
`/repo/../other` starts with `/repo` as plain text, so the disjoint-path check read it as living inside the worktree and let a foreign session keep the pane it had inherited. Collapse `.` and `..` lexically first, for POSIX and drive-rooted paths alike, with an over-popping prefix landing on the root rather than escaping it.
|
@coderabbitai review The docstring coverage check still reports the value from the first commit. Every function added in |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
commit="e2488d1b3"
git rev-parse --verify "${commit}^{commit}"
printf '\n== Changed files at %s ==\n' "$commit"
git diff-tree --no-commit-id --name-status -r "$commit"
printf '\n== Outline: cwd attribution utility ==\n'
git show "${commit}:src/shared/agent-hook-cwd-attribution.ts" > /tmp/agent-hook-cwd-attribution.ts
ast-grep outline /tmp/agent-hook-cwd-attribution.ts --items all || true
nl -ba /tmp/agent-hook-cwd-attribution.ts | sed -n '1,240p'
printf '\n== Outline: hook server ==\n'
git show "${commit}:src/main/agent-hooks/server.ts" > /tmp/agent-hooks-server.ts
ast-grep outline /tmp/agent-hooks-server.ts --items all --match 'AgentHookServer|shouldDrop|cwd|worktree' || true
rg -n -i -C 8 'cwd|worktree|drop|attribution|docstring' /tmp/agent-hooks-server.ts
printf '\n== Docstring-check configuration and scripts ==\n'
rg -n -i -C 3 'docstring.{0,30}(coverage|check)|coverage.{0,30}docstring|typedoc|documentation coverage' \
-g 'package.json' -g '*config*' -g '*.json' -g '*.yaml' -g '*.yml' -g '*.js' -g '*.ts' \
. 2>/dev/null | head -n 300Length of output: 2183 🏁 Script executed: #!/bin/bash
set -euo pipefail
commit="e2488d1b3"
parent="$(git rev-parse "${commit}^")"
printf '%s\n' '== src/shared/agent-hook-cwd-attribution.ts at requested commit =='
git show "${commit}:src/shared/agent-hook-cwd-attribution.ts" | sed -n '1,220p'
printf '%s\n' '== Relevant AgentHookServer declarations at requested commit =='
git show "${commit}:src/main/agent-hooks/server.ts" \
| grep -n -i -C 12 -E 'cwd|worktree|drop|attribution' \
| head -n 260
printf '%s\n' '== Commits that introduced/changed server cwd attribution code (up to requested commit) =='
git log --format='%H %s' "${commit}" -- src/main/agent-hooks/server.ts | head -n 15
printf '%s\n' '== Diff for server attribution change from the immediate parent =='
git diff "${parent}" "${commit}" -- src/main/agent-hooks/server.ts || true
printf '%s\n' '== Documentation / coverage configuration candidates =='
find . -maxdepth 3 -type f \
\( -name 'package.json' -o -name '*config*' -o -name '*.json' -o -name '*.yaml' -o -name '*.yml' -o -name '*.js' -o -name '*.ts' \) \
-not -path '*/node_modules/*' -print0 \
| xargs -0 grep -n -i -E 'docstring.{0,40}(coverage|check)|coverage.{0,40}docstring|typedoc|documentation coverage' \
2>/dev/null \
| head -n 300 || true
printf '%s\n' '== JSDoc-style comments adjacent to functions in the utility file =='
git show "${commit}:src/shared/agent-hook-cwd-attribution.ts" \
| grep -n -E -C 3 '(^/\*\*|function |export function )'Length of output: 17688 🏁 Script executed: #!/bin/bash
set -euo pipefail
commit="e2488d1b3"
server_file="/tmp/agent-hooks-server-${commit}.ts"
git show "${commit}:src/main/agent-hooks/server.ts" > "$server_file"
printf '%s\n' '== hookCwdContradictsWorktree call sites and enclosing method =='
grep -n -C 8 'hookCwdContradictsWorktree' "$server_file" || true
awk '
/hookCwdContradictsWorktree/ { start=NR-35; if (start<1) start=1; end=NR+45 }
NR >= start && NR <= end { printf "%d:%s\n", NR, $0 }
' "$server_file"
printf '%s\n' '== JSDoc comments immediately associated with private cwd/worktree methods =='
awk '
/\/\*\*/ { comment=$0; incomment=1; next }
incomment { comment=comment "\n" $0; if ($0 ~ /\*\//) incomment=0; next }
/private .*cwd|private .*Cwd|private .*worktree|private .*Worktree|private .*attribution|private .*Attribution/ {
printf "LINE %d\n%s\n%s\n\n", NR, comment, $0
}
' "$server_file"
printf '%s\n' '== Relevant package scripts and tooling configuration =='
find . -maxdepth 3 -type f \
\( -name 'package.json' -o -name 'biome.json' -o -name 'biome.jsonc' -o -name 'eslint.config.*' -o -name '.eslintrc*' -o -name 'typedoc.json' -o -name 'vitest.config.*' \) \
-not -path '*/node_modules/*' \
-print \
-exec sed -n '/docstring/Ip;/typedoc/Ip;/documentation/Ip;/scripts/,/}/p' {} \;
printf '%s\n' '== Diff that introduced the server cwd attribution implementation =='
git log -S'hookCwdContradictsWorktree' --format='%H %P %s' "${commit}" -- src/main/agent-hooks/server.tsLength of output: 20850
|
|
@coderabbitai Docstring coverage moved 40% → 50% after Asking because every function and method added to source in this PR now carries one — the five in |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
commit="897e9c384"
git rev-parse --verify "${commit}^{commit}"
printf '%s\n' "== Repository guidance =="
if [ -f AGENTS.md ]; then
nl -ba AGENTS.md | sed -n '1,260p'
else
fd -HI '^AGENTS\.md$' . -x sh -c 'echo "== $1 =="; nl -ba "$1" | sed -n "1,260p"' sh {} \;
fi
printf '%s\n' "== Files changed through requested commit (PR-relevant) =="
git diff --name-status "$(git merge-base "${commit}" HEAD 2>/dev/null || git rev-parse "${commit}^")" "${commit}" -- \
src/shared/agent-hook-cwd-attribution.ts \
src/main/agent-hooks/server.ts \
src/shared/agent-hook-cwd-attribution.test.ts \
src/main/agent-hooks/server-cwd-attribution.test.ts || true
printf '%s\n' "== Docstring coverage configuration/implementation candidates =="
rg -n -i -C 4 \
'docstring|jsdoc|documentation.{0,30}coverage|coverage.{0,30}documentation|undocumented' \
-g 'package.json' -g 'AGENTS.md' -g '*config*' -g '*.json' -g '*.jsonc' -g '*.yaml' -g '*.yml' \
-g '*.js' -g '*.cjs' -g '*.mjs' -g '*.ts' . | head -n 500
printf '%s\n' "== Package scripts =="
fd -HI '^package\.json$' . -x sh -c 'echo "== $1 =="; jq -r ".scripts // {} | to_entries[] | \"\(.key): \(.value)\"" "$1"' sh {} \;
printf '%s\n' "== Requested commit: source and test function outlines =="
for path in \
src/shared/agent-hook-cwd-attribution.ts \
src/main/agent-hooks/server.ts \
src/shared/agent-hook-cwd-attribution.test.ts \
src/main/agent-hooks/server-cwd-attribution.test.ts
do
if git cat-file -e "${commit}:${path}" 2>/dev/null; then
tmp="/tmp/$(basename "$path")"
git show "${commit}:${path}" > "$tmp"
echo "== ${path} =="
ast-grep outline "$tmp" --items all || true
fi
done
printf '%s\n' "== JSDoc/function adjacency in PR files at requested commit =="
for path in \
src/shared/agent-hook-cwd-attribution.ts \
src/main/agent-hooks/server.ts \
src/shared/agent-hook-cwd-attribution.test.ts \
src/main/agent-hooks/server-cwd-attribution.test.ts
do
if git cat-file -e "${commit}:${path}" 2>/dev/null; then
echo "== ${path} =="
git show "${commit}:${path}" | nl -ba | rg -n -C 3 '\/\*\*|(^|[[:space:]])(export )?(async )?function\b|=>|shouldSuppressForeignCwdStatus'
fi
doneLength of output: 2634 🏁 Script executed: #!/bin/bash
set -euo pipefail
commit="897e9c384"
numbered() {
awk '{ printf "%6d\t%s\n", NR, $0 }' "$1"
}
printf '%s\n' "== Repository guidance =="
if [ -f AGENTS.md ]; then
numbered AGENTS.md | sed -n '1,300p'
else
fd -HI '^AGENTS\.md$' . -x sh -c 'echo "== $1 =="; awk "{ printf \"%6d\\t%s\\n\", NR, \$0 }" "$1" | sed -n "1,300p"' sh {} \;
fi
printf '%s\n' "== Documentation-check candidates =="
rg -n -i -C 4 \
'docstring|jsdoc|documentation.{0,30}coverage|coverage.{0,30}documentation|undocumented' \
-g 'package.json' -g 'AGENTS.md' -g '*config*' -g '*.json' -g '*.jsonc' -g '*.yaml' -g '*.yml' \
-g '*.js' -g '*.cjs' -g '*.mjs' -g '*.ts' . | head -n 600 || true
printf '%s\n' "== Package scripts =="
fd -HI '^package\.json$' . -x sh -c \
'echo "== $1 =="; grep -n -i -C 2 -E "\"(doc|string|jsdoc|test|lint|check)'"'"'" "$1" || true' sh {} \;
printf '%s\n' "== PR files at requested commit: AST outlines =="
for path in \
src/shared/agent-hook-cwd-attribution.ts \
src/main/agent-hooks/server.ts \
src/shared/agent-hook-cwd-attribution.test.ts \
src/main/agent-hooks/server-cwd-attribution.test.ts
do
if git cat-file -e "${commit}:${path}" 2>/dev/null; then
tmp="/tmp/$(basename "$path")"
git show "${commit}:${path}" > "$tmp"
echo "== ${path} =="
ast-grep outline "$tmp" --items all || true
fi
done
printf '%s\n' "== JSDoc and candidate declarations in PR files at requested commit =="
for path in \
src/shared/agent-hook-cwd-attribution.ts \
src/main/agent-hooks/server.ts \
src/shared/agent-hook-cwd-attribution.test.ts \
src/main/agent-hooks/server-cwd-attribution.test.ts
do
if git cat-file -e "${commit}:${path}" 2>/dev/null; then
echo "== ${path} =="
git show "${commit}:${path}" \
| awk '{ printf "%6d\t%s\n", NR, $0 }' \
| rg -n -C 3 '\/\*\*|(^|[[:space:]])(export )?(async )?function\b|=>|shouldSuppressForeignCwdStatus' \
|| true
fi
doneLength of output: 43859 🏁 Script executed: #!/bin/bash
set -euo pipefail
tip="897e9c384"
base="$(git merge-base "$tip" origin/main 2>/dev/null || git merge-base "$tip" main 2>/dev/null || git rev-parse "${tip}^")"
printf '== Range ==\nbase=%s\ntip=%s\n' "$base" "$tip"
git log --format='%h %s' "${base}..${tip}"
printf '\n== Changed TypeScript files ==\n'
git diff --name-status "$base" "$tip" -- '*.ts'
printf '\n== Added lines that introduce or invoke callable expressions ==\n'
git diff --unified=4 "$base" "$tip" -- '*.ts' \
| awk '
/^\+\+\+ / { file=$2; sub("^b/", "", file); next }
/^\+/ && !/^\+\+\+/ &&
($0 ~ /(function[[:space:]]+[A-Za-z_$]|[A-Za-z_$][A-Za-z0-9_$]*[[:space:]]*[:=][[:space:]]*(async[[:space:]]*)?\([^)]*\)[[:space:]]*=>|=>|describe\(|it\(|beforeEach\(|afterEach\(|vi\.mock\()/) {
print "FILE " file "\n" $0
}' \
| sed -n '1,600p'
printf '\n== Full PR diffs for the new attribution source/tests ==\n'
git diff --unified=6 "$base" "$tip" -- \
src/shared/agent-hook-cwd-attribution.ts \
src/shared/agent-hook-cwd-attribution.test.ts \
src/main/agent-hooks/server-cwd-attribution.test.ts \
src/main/agent-hooks/server.ts \
src/relay/agent-hook-server.test.ts \
src/relay/agent-hook-server.ts \
src/shared/agent-hook-listener.ts \
src/shared/agent-hook-relay.ts \
src/shared/telemetry-events.ts | sed -n '1,2200p'
printf '\n== Function-like nodes in newly added test files at tip ==\n'
for path in src/shared/agent-hook-cwd-attribution.test.ts src/main/agent-hooks/server-cwd-attribution.test.ts; do
echo "== $path =="
git show "${tip}:${path}" > "/tmp/$(basename "$path")"
ast-grep run --lang ts --pattern '$FUNC($$$ARGS) => { $$$BODY }' "/tmp/$(basename "$path")" || true
ast-grep run --lang ts --pattern 'function $FUNC($$$ARGS) { $$$BODY }' "/tmp/$(basename "$path")" || true
doneLength of output: 31841
The only added callables without JSDoc are test/mocking callbacks:
That is 21 undocumented callback expressions total. There are no remaining undocumented production functions in the PR. So, if the metric still reports only 50%, it is either including these test callbacks (and possibly nested callback expressions) or is stale. I agree that adding boilerplate JSDoc to 🐇🔎 ✏️ Learnings added
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ee837220-5d23-43c6-8be7-406c11e7d713
📒 Files selected for processing (9)
src/main/agent-hooks/server-cwd-attribution.test.tssrc/main/agent-hooks/server.tssrc/relay/agent-hook-server.test.tssrc/relay/agent-hook-server.tssrc/shared/agent-hook-cwd-attribution.test.tssrc/shared/agent-hook-cwd-attribution.tssrc/shared/agent-hook-listener.tssrc/shared/agent-hook-relay.tssrc/shared/telemetry-events.ts
…ation `C:\..\..` collapsed to a slashless `c:`, which no longer matched the drive-rooted test. A worktree or cwd in that shape would then be compared against a POSIX path as if both used the same notation, and the mixed-notation bail-out that keeps unclear pairs attributed could not fire — the one way this guard could drop a legitimate status row. Terminate collapsed drive roots with their slash and let the containment check accept any root-terminated prefix.
|
@coderabbitai full review Last full round covered up to |
|
🐇🔍 ✅ Action performedFull review finished. |
The guard hand-rolled path normalization that `cross-platform-path.ts` already provides. Two of this branch's earlier commits fixed bugs the reuse never had, and the copy missed NFC folding, so a non-ASCII workspace path dropped every status from that workspace (stablyai#10832). Delegate to `resolveRuntimePath` and `isPathInsideOrEqual`, keep the notation bails as guard policy, and drop `readHookPayloadCwd` in favour of `readBoundedString` — which also restores the 4096 cwd bound the hand-rolled reader lost. Stop persisting `sourceCwd`, a transport-only field the hydrate whitelist drops.
|
@coderabbitai full review The path-comparison logic was replaced wholesale in 3989d46. The hand-rolled normalizer is gone in favour of Notable behaviour changes since the last review:
|
|
✏️ Learnings added
✅ Action performedFull review finished. |
The cwd guard emitted telemetry on every dropped hook. A daemon that mis-attributes one pane fires on every hook of every session it hosts, and the per-session telemetry ceiling is shared by all events and never refills — so the guard could silence the rest of the session's telemetry within the hour. Move the track call inside the warn-once gate that already existed beside it; `reason` carries no pane, so repeat events add no signal.
|
@coderabbitai full review Re-requesting against the current head
Worth checking in particular: whether once-per-runtime is the right granularity given |
|
✏️ Learnings added
✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/agent-hooks/server-cwd-attribution.test.ts (1)
97-118: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover telemetry reset across
stop()/start().This verifies the per-runtime cap but not the required reset. Add a mismatch →
stop()→start()→ mismatch case and expect two telemetry calls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e6c77993-5d6f-40a7-aa71-cff701f9bd23
📒 Files selected for processing (10)
src/main/agent-hooks/server-cwd-attribution.test.tssrc/main/agent-hooks/server.test.tssrc/main/agent-hooks/server.tssrc/relay/agent-hook-server.test.tssrc/relay/agent-hook-server.tssrc/shared/agent-hook-cwd-attribution.test.tssrc/shared/agent-hook-cwd-attribution.tssrc/shared/agent-hook-listener.tssrc/shared/agent-hook-relay.tssrc/shared/telemetry-events.ts
The once-per-runtime cap was covered but the `stop()` reset that lifts it was not, so removing that line failed no test. Add a restart case, and fold the thrice-repeated relay envelope into one local builder.
|
Addressed the nitpick in The per-runtime cap was covered but the I used |
Conflicts in src/main/agent-hooks/server.ts — main's launch-token authority layer landed on the same four sites as this branch's cwd guard. Kept both: the cwd guard runs first on each ingest path so a disproven hook never records an authority observation, and serializeStatusFile now strips sourceCwd alongside promptInteractionKey before hashing launchToken. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EnMabSU7QxCeHwtropbTUq
Greptile SummaryThis PR fixes mis-attributed agent status rows caused by shared daemon processes inheriting the pane identity (
Confidence Score: 5/5
|
| Filename | Overview |
|---|---|
| src/shared/agent-hook-cwd-attribution.ts | New module implementing the core cwd-vs-worktree contradiction logic. Two exported functions: hookCwdContradictsWorktree (pure string, no I/O) and hookCwdContradictsWorktreeAfterLocalResolve (adds symlink resolution via existsSync/realpathSync.native, plus macOS firmlink folding). Edge cases are comprehensively handled: UNC paths, mixed POSIX/Windows notation, WSL, relative paths, dot-segment traversal, NFC normalization, and folder-workspace instance suffixes. Design is conservative — unclear comparisons return false (keep). |
| src/main/agent-hooks/server.ts | Adds shouldSuppressForeignCwdStatus to AgentHookServer. The guard fires in two places on the local HTTP path: before normalization (preventing listener state pollution) and again after normalization as drift armor. For ingestRemote, only the raw string check runs since Orca cannot resolve the relay host's paths. sourceCwd is excluded from last-status.json persistence and the PersistedAgentHookEventPayload type. Telemetry is capped to once-per-runtime via a single boolean reset in stop(). Warning set is bounded to FOREIGN_CWD_WARN_PANE_CAP and pre-sliced before retention. |
| src/relay/agent-hook-server.ts | Adds pre-normalization refusal at the relay HTTP boundary using hookCwdContradictsWorktreeAfterLocalResolve. The applyEvent funnel clears sourceCwd when the raw check detects a contradiction that was already judged keep-worthy by the local-resolve check (symlink aliasing) — this prevents Orca's downstream raw guard from re-dropping a proven-legitimate row or poisoning the replay cache. Warning is per-pane once, bounded to FOREIGN_CWD_WARN_PANE_CAP, and cleared on stop(). |
| src/shared/agent-hook-listener.ts | Adds readHookBodyCwdAttribution (pre-normalization read of paneKey, worktreeId, sourceCwd) and populates sourceCwd in normalizeHookPayload output. Renames GROK_SESSION_CWD_MAX_LENGTH to HOOK_CWD_MAX_LENGTH and exports it; unifies HOOK_CWD_KEYS constant used by both the attribution reader and the Grok session metadata reader. The sourceCwd field is documented as transport-only on AgentHookEventPayload with a comment explaining the guard invariant. |
| src/relay/agent-hook-envelope-build.ts | One-line addition: sourceCwd is forwarded in the built relay envelope. The comment correctly explains that applyEvent has already sanitized the value before this function is reached, so both live and replay legs are safe. |
| src/main/ssh/ssh-relay-session.ts | Passes sourceCwd from the relay envelope through to ingestRemote with a loose type check (string or undefined). Length bounding is enforced at the ingestRemote trust boundary in server.ts, not here — consistent with how other relay-forwarded fields like providerSession are handled. |
| src/shared/telemetry-events.ts | Adds 'cwd_worktree_mismatch' to the agentHookUnattributed reason enum. The schema remains strict (closed enum), so no identifying string (path, repo name) enters telemetry — consistent with existing constraints on this event. |
Sequence Diagram
sequenceDiagram
participant Agent as Agent CLI (shared daemon)
participant RelayHTTP as Relay HTTP Server
participant OrcaHTTP as Orca Hook HTTP Server
participant ingestRemote as Orca ingestRemote
participant Status as Last-Status Store
Note over Agent: Inherits pane env from first session<br/>(ORCA_PANE_KEY, ORCA_WORKTREE_ID)
Agent->>RelayHTTP: "POST /hook/claude {paneKey, worktreeId, payload.cwd}"
RelayHTTP->>RelayHTTP: readHookBodyCwdAttribution(body)
RelayHTTP->>RelayHTTP: "hookCwdContradictsWorktreeAfterLocalResolve()<br/>(existsSync + realpathSync.native)"
alt cwd disproves worktree (disjoint paths, no symlink alias)
RelayHTTP-->>Agent: 204 (dropped)
RelayHTTP->>RelayHTTP: warnForeignCwdOnce() → stderr
else cwd consistent (or symlink alias resolved)
RelayHTTP->>RelayHTTP: "normalizeHookPayload()<br/>applyEvent(): clears sourceCwd if<br/>raw contradiction survived local resolve"
RelayHTTP->>ingestRemote: forward envelope + sourceCwd
ingestRemote->>ingestRemote: re-validates sourceCwd length
ingestRemote->>ingestRemote: "shouldSuppressForeignCwdStatus()<br/>hookCwdContradictsWorktree (no fs access)"
alt contradicts (remote paths, raw comparison)
ingestRemote-->>Agent: (dropped)
ingestRemote->>ingestRemote: "track('agent_hook_unattributed')<br/>once per runtime"
else consistent
ingestRemote->>Status: "applyNormalizedStatus()<br/>persists without sourceCwd"
end
end
Agent->>OrcaHTTP: POST /hook/claude (local HTTP path)
OrcaHTTP->>OrcaHTTP: "readHookBodyCwdAttribution(body)<br/>shouldSuppressForeignCwdStatus(resolve=true)"
alt drops
OrcaHTTP-->>Agent: 204 (dropped, telemetry emitted)
else keeps
OrcaHTTP->>OrcaHTTP: "normalizeLocalHookPayload()<br/>drift-armor re-check"
OrcaHTTP->>Status: "applyNormalizedStatus()<br/>persists without sourceCwd"
end
Reviews (5): Last reviewed commit: "Merge upstream/main into fix/agent-hook-..." | Re-trigger Greptile
The mux-notification handler rebuilds the ingestRemote envelope field by field and omitted sourceCwd, leaving the cwd attribution guard inert for every SSH event (live and replay).
…olving symlink aliases - Re-judge a would-drop on locally resolved paths so one directory spelled two ways (macOS /tmp vs /private/tmp, symlinked roots) is not read as a foreign session; the relay strips sourceCwd it proved alias-clean so Orca's raw re-guard cannot re-drop the row. - Run the guard before normalizeHookPayload on both local and relay HTTP ingest so a foreign daemon-hosted session cannot seed per-pane subagent rosters or the replay cache that later legitimate events re-emit. - Warn per pane (bounded) instead of once per runtime; telemetry stays latched. Exclude sourceCwd from the persisted payload type.
…oint - The assistant-message retry and codex subagent poll re-normalize the raw body and re-attached the contradicting cwd, so main's raw re-guard dropped enrichment updates and poisoned the replay cache for symlink-aliased remote workspaces; move the strip into applyEvent, which every cache/forward leg funnels through. - Keep, not drop, when a local path exists but cannot be resolved, and fold the macOS data-volume firmlink prefix — both only rescue rows. - Bound warn-set keys and logged values on both hook servers. - Pin the retry-leg strip, the relay guard-before-normalize ordering, and cwd-side symlink resolution with discriminating tests.
…ion is stat-able locally A recorded worktree spelling that cannot be stat-ed on its own host (renamed, deleted, whitespace-mangled by the transit trim) cannot prove a foreign session; both-nonexistent still drops so foreign-host verdicts stand. Pins the exists-but-unresolvable keep with deterministic fs stubs.
One-side-stat-able and unresolvable keeps also survive the refusal now; the surviving raw contradiction is host-judged keep-worthy, not always symlink aliasing.
Five files conflicted, all where upstream's claudeRunningNonAgentTask work and
this branch's sourceCwd work touch the same lists and literals. Both sides kept
everywhere; the only non-additive resolution is the local HTTP ingest, where
upstream replaced the bare normalizeHookPayload call with
normalizeLocalHookPayload returning { event, onAccepted }. The pre-normalization
cwd refusal and the post-normalization drift guard were re-applied on top of the
new shape, so the guard still runs before listener state is seeded.
In src/relay/agent-hook-server.test.ts the two sides' new tests overlapped as one
block; upstream's background-work test and this branch's four cwd tests are both
present.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HL8CxXZYghUKJEQpYhKeGg
# Conflicts: # src/main/agent-hooks/server.ts
# Conflicts: # src/main/agent-hooks/server.ts
# Conflicts: # src/relay/agent-hook-server.ts
|
Conflicts resolved against current main in
Verified after installing against this branch's lockfile: Still sequenced behind #14615 as @brennanb2025 described — this is only to keep the branch mergeable while that lands. |
Two things main moved under this branch. `src/main/agent-hooks/server.test.ts` was split across many `server-*.test.ts` files (stablyai#14728). This branch's only edit to it was one last-status test, which now lives in `server-last-status-write.test.ts` — moved there verbatim, including the `cwd` field and the `sourceCwd` assertion. `src/shared/worktree-id.ts` became `src/shared/worktree/id.ts` (stablyai#14437). The two files this branch adds import three symbols from it and were the only references the merge did not resolve; nothing else about them changed. Verified in this worktree after installing against the branch lockfile: tsc clean on all three projects, oxlint clean, and `src/main` plus `src/shared` at 2551 files / 26541 tests passing.
cfd2c34 to
fd9fe14
Compare
One conflicting file, src/relay/agent-hook-server.ts, and the conflict is entirely stablyai#14725's split of that file: main moved the endpoint coordinates, the envelope builder and the assistant-message/codex retry scheduling out into three modules, so every constant, import and method this branch had added sat next to something that had left. Resolved by keeping only what the merged file still reaches: the cwd attribution imports and FOREIGN_CWD_WARN_PANE_CAP stay, the retry and endpoint constants go with the code that moved, and the class keeps both sides' fields. The one resolution git could not have reached on its own is in a file it reported as clean. This branch added `sourceCwd` to the envelope inside the private `forwardEvent`; stablyai#14725 extracted that method into buildRelayHookEnvelope in agent-hook-envelope-build.ts, before the field existed, and repointed both the live and replay call sites at it. Taking either side alone drops the field silently — the method is dead with no callers, and the builder never carried it. `sourceCwd` now lives in the builder, which is what both legs go through. Verified by removing that one line again: 'forwards a parsed Claude UserPromptSubmit POST as a normalized envelope' fails with `expected undefined to be '/srv/app'`. relay, shared agent-hook and main/agent-hooks suites: 197 files, 2010 passing. tsc clean on the node project, oxlint and oxfmt clean, 85 reliability gates, ratchet at 190.
Three additions landing on the same lines, none of them touching what this branch decides. `observation` joined `sourceCwd` on the never-persisted list, and its destructure now reads main's `enrichedPayload` so the child-only boundary flag survives a write. The retired-pane fence helpers sit beside the cwd guard rather than in place of it. The guard's three call sites are unchanged: relay ingest, the pre-validation HTTP check, and the normalized local event.
|
Closing this PR as superseded by merged PR #14615, which now owns the same agent-hook pane/session cwd-attribution behavior on current main. We will not merge this older branch. The useful diagnosis and implementation work are retained in our owner follow-up, with full credit to @kunsanglee. No action is needed from you. Thank you for the contribution. |
|
Correction to my previous comment: I closed this PR prematurely after treating merged PR #14615 as a complete replacement. The two changes are complementary: #14615 handles spawn-time session pinning, while this PR provides a defensive cwd-versus-pane attribution guard for sessions that cannot be pinned. I have reopened this PR and restored it to our owner queue. We will evaluate and, if warranted, reimplement/port the guard ourselves on current main; no action is requested from you. Full credit to @kunsanglee is preserved. Apologies for the incorrect closure. |
|
Closing this submitted branch and routing the bug to canonical owner issue #17578. We will carry the work ourselves on current main and will not request contributor follow-up. Full contributor credit is preserved for @kunsanglee. No action is requested from you. Thank you. |
3 participants
Summary
A session running in one workspace can have its agent status land on a different workspace's sidebar card, while the workspace it actually runs in shows nothing.
Agent CLIs increasingly pre-warm a shared background daemon and hand later sessions to it. The daemon keeps the environment of whichever pane spawned it, including
ORCA_PANE_KEY,ORCA_TAB_ID, andORCA_WORKTREE_ID. Every session it hosts afterwards inherits that first pane's Orca identity, and the managed hook script posts those env values verbatim. When two sessions land on the same pane key the server merges their fields into one row, so the visible row mixes one session's prompt with another's subagents.Observed on 1.4.158 with Claude Code: one
claude daemon runprocess spawned from a workspace pane at 09:23, and every session claimed from its pre-warmed spares carried that pane'sORCA_PANE_KEY.last-status.jsonheld a single entry keyed to that pane whose transcript path pointed at an entirely different project, and the pane actually running that project had no entry at all.This change makes the hook server verify pane attribution instead of trusting inherited env.
normalizeHookPayloadnow keeps the session cwd the agent reports in its own payload, and the server cross-checks it against the worktree path already encoded in the reportedworktreeId(<repoId>::<path>). When the two are disjoint the event is refused rather than written to the wrong pane, andagent_hook_unattributedis tracked with a newcwd_worktree_mismatchreason.Ambiguity keeps today's behavior, since dropping a real status row is the worse failure:
/mnt/c/…WSL cwd, or a UNC path on either side, since UNC aliases the other notations (\\wsl$\Ubuntu\mnt\c\xisC:\x, and\\server\sharecan be a mapped drive)Path comparison itself is delegated to
src/shared/cross-platform-path.ts, the repository's canonical path layer:resolveRuntimePathcollapses.and..before any containment test, so a cwd like/repo/../othercannot pose as living inside/repo, andisPathInsideOrEqualfolds NFC so an NFD workspace path from the folder picker still matches the NFC cwd an agent reports (#10832). No filesystem access is involved.The relay forwards
sourceCwdbeside the normalized payload, because payload normalization strips cwd, so remote sessions get the same check.sourceCwdstays transport-only and is stripped beforelast-status.jsonis written, matching howpromptInteractionKeyis already handled — the hydrate whitelist does not read it back, and persisting it would make hydration lossy and defeat the write dedupe that assumes losslessness.Screenshots
No visual change in the sense of UI code: no renderer file is touched, and no layout, token, or component changes. The user-visible effect is which sidebar card an agent row appears on. I did not attach a capture of the failure because reproducing it on screen requires a daemon already holding another pane's environment, and my only capture of it is a personal workspace with unrelated private content.
Testing
pnpm lintpnpm typecheckpnpm testpnpm buildNotes on how these were run and what they showed:
pnpmis not installed on this machine, so each gate was run through its underlying commands frompackage.jsonrather than the wrapper: all fouroxlintstages including both--type-awareconfigs, then every check script in thelintchain (styled scrollbars, quadratic buffer concat, reliability gates, max-lines ratchet, bundled skill guides, skill bundle manifest, localization catalog and coverage). All passed. Typecheck ran all three projects (node,cli,web). Build ranrelay,cli,electron-vite, andweb-from-rendererto completion.build:nativeshells out topnpmitself, so I ran its two macOS targets directly —build-computer-macos.mjsandbuild-notification-status-macos.mjs— and both linked and signed successfully.pnpm test: 3,658 files pass, 10 fail (49 assertions). Every failure is a renderer file failing onwindow.localStorage.clear is not a functionin the test environment. These are pre-existing: I checked out cleanupstream/main(89968a106) and ran the same files there, where they fail identically. The exact set of affected renderer files shifts between runs — a second full run reported 12 files and 51 assertions — which fits an environment problem rather than anything in this change. Nothing this PR touches fails:src/main/agent-hooks,src/shared/agent-hook*, andsrc/relaypass in full at 1,544 tests.src/shared/agent-hook-cwd-attribution.test.tscovers the comparison contract: containment in both directions, the mixed-notation and UNC refusals, dot segments that resolve outside the worktree, drive roots, folder-workspace instance ids, and an NFD-vs-NFC non-ASCII path that must not read as a conflict.src/main/agent-hooks/server-cwd-attribution.test.tscovers the local HTTP path, the relay path, the no-cwd passthrough that must stay attributed, and the once-per-runtime telemetry cap together with thestop()reset that lifts it.src/relay/agent-hook-server.test.tsasserts the relay forwards the cwd it parsed, which is the field the remote check depends on.src/main/agent-hooks/server.test.tsassertssourceCwdnever reacheslast-status.json. Each of these was confirmed to fail with its production change reverted before being kept.AI Review Report
Reviewed with Claude Code, including a pass dedicated to reuse and simplification after the first implementation was complete. That pass is what produced the delegation to
cross-platform-path.tsdescribed below, and it is worth stating plainly: two of this branch's earlier commits fixed boundary bugs that only existed because the path logic had been hand-rolled instead of reused.src/shared/cross-platform-path.ts, which the repository already uses at hundreds of call sites, rather than reimplemented:isRuntimePathAbsolutegates unrooted paths,isWindowsAbsolutePathLikeseparates drive-rooted from POSIX notation,resolveRuntimePathcollapses dot segments, andisPathInsideOrEqualperforms containment with NFC folding and correct drive-root boundaries. The one deliberate deviation is that this guard case-folds POSIX paths, which the shared comparison helper declines to do because POSIX filesystems are byte-exact. The asymmetry is the reason: the shared helper's callers use it to pick candidates, where a missed match costs nothing, while here a missed match drops a live status row. The reason is stated in a comment at the call site. Drive-rooted and POSIX paths are never compared against each other, and a UNC path on either side bails, which together leave the guard inert on WSL; that is also stated in a comment, since it is a real coverage gap rather than an oversight. No shortcut, label, menu accelerator, or Electron platform surface is touched. Comparison is pure string work with no filesystem access.ingestRemotepath are both guarded. Since payload normalization strips cwd before the envelope is built, the relay had to forward it explicitly or the remote check would have silently never fired — that was caught during review and fixed, with a relay test pinning it. Remote worktree paths and remote cwds are both remote-filesystem paths, so they compare on equal terms. The guard is applied at the two entry points that can carry asourceCwdrather than at the sharedapplyNormalizedStatusfunnel: four of that function's six callers cannot structurally carry the field, so funnelling would mean a nullable return threaded through six call sites in exchange for four dead checks. The invariant is instead recorded on the field declaration, which is what a future third entry point has to touch.readBoundedStringover the same['cwd', 'workspaceRoot', 'workspace_root']key list the Grok session reader already uses, so the two cannot drift and the established 4096-character bound applies. WhenworkspaceRootnames a project root rather than the session directory it sits at or above the worktree, which the nested-path allowance treats as consistent.createNormalizedPathInsideOrEqualMatcherto avoid renormalizing was built and measured, and was slower in the common case, so it was not adopted. No new allocation in any hot loop, no I/O, no new timers or listeners.Security Audit
sourceCwdcrosses the SSH trust boundary on the relay path, soingestRemotetype-checks it, bounds its length against the same 4096-character limit the local path enforces, trims it, and treats empty as absent. On the local path the value comes from the same hook payload the server already parses, through the listener's existing bounded reader. It is only ever compared as a string.stat, norealpath, no reads or writes driven by the value. The value is normalized only for comparison, so a hostile path cannot cause traversal, and it is never used to build a filesystem operation. Traversal segments are collapsed before comparison, which closes the one way a crafted cwd could have kept a pane it does not own.cwd_worktree_mismatchreason is a closed enum value. No path, repo name, or other identifying string enters telemetry, matching the existing constraint on that event's schema. It is emitted once per runtime rather than once per dropped hook: a mis-attributing daemon fires on every hook of every session it hosts, and the per-session ceiling insrc/main/telemetry/burst-cap.tsis shared across all event names and never refills, so an uncapped emit here could have exhausted it and silenced the rest of the session's telemetry. Sincereasoncarries no pane, repeat events add no signal. A test pins both the cap and its reset instop().console.warn, once per runtime, includes the pane key, worktree id, and cwd. It goes to the local Electron console only. This is a deliberate choice: without those three values the warning cannot be acted on, and the same process already persists transcript paths locally.sourceCwdis excluded fromlast-status.json. It is transport-only, the hydrate whitelist never reads it back, and keeping it out means no absolute path is added to what that file already stores.stop(), so it cannot grow.Notes
cd-ing outside that workspace now drops its status instead of showing it on that pane's card. At the hook boundary this is indistinguishable from the inherited-env case: both are a foreign cwd arriving with a valid pane's env. Refusing is the safer of the two, but it is a real change for that workflow.toAgentStatusIpcPayloadnot carryingsourceCwdis the one-line seam where that follow-up would start, since the renderer already has a resolver for "which worktree owns this session cwd".process.env.ORCA_PANE_KEYand act on it, for examplesrc/cli/handlers/orchestration.tsresolving a pane to write to andsrc/cli/handlers/worktree.tspicking a parent workspace, with no server-side cross-check. Those are arguably more consequential than a misplaced sidebar row, and each already hasprocess.cwd()in hand, so the predicate added here would apply directly. It is a separate change and I have not made it. Flagging it here rather than leaving it undocumented.PANE_IDENTITY_ENV_KEYS, but it cannot reach a daemon it does not spawn, which is why the fix is verification on receipt. Detecting rather than preventing inherited pane identity also matches the existing precedent insrc/shared/agent-status-identity.ts, which handles nested child hooks inheriting a parent'sORCA_PANE_KEYthe same way.describe/it/mock/beforeEachcallbacks. Adding JSDoc to those would conflict withAGENTS.md, which asks for concise non-obvious comments rather than restatements, so I left them alone; CodeRabbit reviewed and agreed in the thread.X (Twitter): @kunsanglee
🤖 Generated with Claude Code