Skip to content

fix(advisor): surface nested repo context#3156

Open
oldschoola wants to merge 1 commit into
can1357:mainfrom
oldschoola:fix/advisor-nested-repo-context
Open

fix(advisor): surface nested repo context#3156
oldschoola wants to merge 1 commit into
can1357:mainfrom
oldschoola:fix/advisor-nested-repo-context

Conversation

@oldschoola

Copy link
Copy Markdown
Contributor

Summary

  • detect a single direct child git repo when the session cwd itself is outside git
  • surface that active repo in the status line, git branch/status/PR lookup, main prompt, and advisor prompt
  • add regression coverage for nested-repo context and Windows-safe status path cleanup

Fixes #3130

Verification

  • HOME="$PWD/.wt/test-home" bun test packages/coding-agent/test/git-active-context.test.ts packages/coding-agent/test/status-line-path.test.ts packages/coding-agent/test/status-line-model.test.ts packages/coding-agent/test/status-line-overflow.test.ts packages/coding-agent/test/issue-953-repro.test.ts packages/coding-agent/test/system-prompt-dedup.test.ts packages/coding-agent/test/advisor-watchdog.test.ts packages/coding-agent/src/advisor/tests/advisor.test.ts
  • bun check

@github-actions github-actions Bot added the vouched Passed the vouch gate label Jun 20, 2026

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: bd3a35e13d

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

const gitStatus = includeGit ? this.#getGitStatus() : null;
const gitPr = includePr ? this.#lookupPr() : null;

const activeRepoCache = this.#resolveActiveRepoCache();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Respect disabled git setting before resolving active repo

When git.enabled is false (or when no path/git/PR segment needs repo context), this unconditional call still runs resolveActiveRepoContextSync(), which invokes git.repo.resolveSync and scans the cwd for repositories on every first status-line render. That breaks the existing status-line-settings-cache.test contract that disabled git integration performs no git probes, and it ignores users who turn git integration off to avoid repo filesystem work. Gate this resolution on the visible segments/settings before calling it.

Useful? React with 👍 / 👎.

@roboomp roboomp added agent Agent runtime planning and orchestration fix prompting Prompt templates and prompt assembly review:p1 triaged tui Terminal UI rendering and display labels Jun 20, 2026
@roboomp

roboomp commented Jun 20, 2026

Copy link
Copy Markdown
Collaborator

review:p1

Thanks for the patch, @oldschoola. The fix tracks the accepted direction in #3130: detect a single direct-child git repo when cwd is outside git, and thread that effective cwd through the status-line git/PR lookups, advisor watchdog prompt, and main system prompt. Conservative trigger, coherent scope (one new utils/active-repo-context.ts, one prompt per consumer, status-line cache keyed on getProjectDir() so the per-render path stays O(1)). Author's claimed verification reproduces locally — 85 pass / 0 fail across the listed files; bun check clean. Incidental win: gh pr view is now correctly scoped via .cwd(lookupCwd) instead of inheriting process.cwd().

(Inline anchors couldn't be flushed as a batched review — one staged comment hit a line outside the diff hunks and the staging surface here is append-only, so the findings live here instead. Sorry for the noise.)

Findings, all should-fix / nit:

  • Perf — packages/coding-agent/src/utils/active-repo-context.ts findSingleDirectChildRepo[Sync]. Once the outer resolver has proven resolveRepository(resolvedCwd) === null, you already know no ancestor of cwd contains a .git. The per-child loop still calls repo.resolve(childPath), which walks each child's ancestors back to the filesystem root looking for .git — wasted I/O on every non-repo sibling. A microbench in a $HOME-like parent with ~300 children measured async resolveActiveRepoContext ≈ 474ms at session start; sync ≈ 2.6ms cached is fine. Shortcut: directly stat childPath/.git (or call a leaf-only variant of resolveRepoFromEntry) instead of re-walking. Bounded to one extra stat per child instead of O(depth-to-root) per child.

  • Windows test — packages/coding-agent/test/git-active-context.test.ts "treats a direct child symlink…" (≈line 86). fs.symlinkSync(targetRoot, repoRoot, "junction") requires admin or Developer Mode on Windows; non-elevated CI will throw and fail. Sibling tests in status-line-path.test.ts already gate Windows with if (process.platform === "win32") return;. Wrap the symlink creation in a try/catch with skip, or guard the whole it(...) body the same way.

  • Code dup — normalizePromptPath. Defined identically in packages/coding-agent/src/system-prompt.ts (~line 92) and packages/coding-agent/src/advisor/watchdog.ts (~line 9), both doing value.replace(/\\/g, "/"). Push down to a shared util (alongside ActiveRepoContext in utils/active-repo-context.ts, or tools/render-utils next to shortenPath). Two copies is the threshold from AGENTS.md ("extract at 2 occurrences, not 5").

  • setCachedPr cwd dimension — packages/coding-agent/src/modes/components/status-line/component.ts ≈line 514 (unchanged here, but newly relevant). The success-path re-resolves the current branch via this.#getCurrentBranch() without passing lookupCwd. Once #activeRepoCache invalidates on the next cwd hop, latestBranch becomes the branch at the new effective cwd while lookupContext.branch is the launch-time branch. The mismatch silently discards the result via isSamePrCacheContext, leaving #cachedPr stale. Was previously masked because gh pr view ran in process.cwd(); now that .cwd(lookupCwd) correctly scopes the subprocess, this is the only place left where launch-cwd-vs-current-cwd matters. Consider this.#getCurrentBranch(lookupCwd) inside setCachedPr. Not blocking.

  • Truncation order — packages/coding-agent/src/modes/components/status-line/segments.ts line 217. The ↳ <relativeRepoRoot> suffix is appended before shortenPath and maxLength truncation, so on narrow status lines the trailing-slice truncation can chop off either the parent prefix or the active-repo indicator. For users with the long-cwd case this PR targets, the active-repo marker is the more interesting signal — consider rendering the suffix after truncation (reserve suffix length, truncate pwd first, then append) so ↳ repo is guaranteed visible. Not blocking; consistent with existing path truncation.

Open question for the maintainer: the advisor watchdog prompt and the system-prompt block both resolve activeRepoContext once at session creation (sdk.ts ≈line 1132 / buildSystemPrompt consumes the pre-resolved value). If a user cds into the child repo or to a different parent mid-session, both prompts go stale until the session is recreated. Matches the existing WATCHDOG.md discovery contract, but worth confirming you're OK with that vs. re-resolving on each system-prompt rebuild.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent Agent runtime planning and orchestration fix prompting Prompt templates and prompt assembly review:p1 triaged tui Terminal UI rendering and display vouched Passed the vouch gate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Advisor false-positive STOP warnings when working in a subdirectory of cwd

2 participants