Skip to content

docs: update context-PR + slice-BRC observability for #2684 [doc-updater] - #2693

Merged
jwbron merged 1 commit into
mainfrom
egg/doc-update-2684-worktree-base-dir
May 12, 2026
Merged

docs: update context-PR + slice-BRC observability for #2684 [doc-updater]#2693
jwbron merged 1 commit into
mainfrom
egg/doc-update-2684-worktree-base-dir

Conversation

@james-in-a-box

Copy link
Copy Markdown
Contributor

Update documentation to reflect changes from b9bff47 (#2690):

  • Add WORKTREE_BASE_DIR missing volume mount as a known cause of context_pr.skipped events, with the specific WARNING log line operators should look for
  • Add a diagnostic note to the per-slice BRC history section: missing BRC files in a slice PR diff can be diagnosed by checking orchestrator logs for the WORKTREE_BASE_DIR missing warning

Both hooks (_open_context_pr_for_pipeline and _commit_slice_brc_history_to_integration_branch) now fall back to /tmp with a warning when WORKTREE_BASE_DIR is absent, rather than silently failing — making this failure mode observable through logs.

Triggered by: #2690

Authored-by: egg

Add WORKTREE_BASE_DIR missing as a known cause of `context_pr.skipped`
and missing slice BRC history files, and document the warning log to
look for when diagnosing a broken volume mount.

Authored-by: egg

@egg-reviewer egg-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Review

Pure documentation update extending docs/reference/orchestrator-cli.md with two operator-facing forensics hints tied to the WORKTREE_BASE_DIR fallback added in #2690 (commit b9bff47).

Verified against source

  • Log-message text matches the code. The quoted Context PR hook: WORKTREE_BASE_DIR missing — falling back to system temp (likely a broken volume mount in production…) (#2684) (line 464) and the slice-BRC counterpart on line 473 are faithful prefixes of the strings at orchestrator/routes/pipelines.py:10285 and :11063 respectively. The trailing correctly elides the same production-context tail in both.
  • /tmp is the right destination to call out. tempfile.mkdtemp(prefix=…, dir=None) resolves through tempfile.gettempdir() and lands at /tmp on the orchestrator pod's Linux container.
  • (gateway-rejected path) parenthetical is correct. gateway/git_client.py:178-183 shows ALLOWED_REPO_PATHS = ["/home/egg/repos/", "/home/egg/.egg-worktrees/", "/home/egg/.egg-state/", "/repos/"]. /tmp/... matches none of them, so validate_repo_path returns (False, "repo_path must be within allowed directories: …") and push_worktree_branch returns ok=False. The hook then logs Context PR hook: push failed, skipping (#2548) at WARNING and returns None without raising — which is exactly the contract-side short-circuit the surrounding paragraph already describes, so categorising this as another context_pr.skipped trigger is accurate.
  • /home/egg/.egg-worktrees path is correct. WORKTREE_BASE_DIR = Path("/home/egg/.egg-worktrees") at orchestrator/routes/pipelines.py:739 (and five sibling modules); the volume-mount framing is consistent with the commit message ("already mounted by the spawner").

Non-blocking notes

  • The doc says "the hook falls back to /tmp" while the in-code log message says "system temp" — both forms are used in the same paragraph. Not worth a follow-up; on Linux they're equivalent and the prose intentionally trades precision for operator readability.
  • The slice-BRC sentence ends a paragraph that previously closed with a "See [BRC-history file naming]" link, so the new sentence reads as an addendum. Fine as-is; if a future edit reshuffles the paragraph, consider moving the forensics hint to its own line or a callout block.

No blocking issues. Both forensics hints are accurate, target a real misconfiguration mode (#2684), and route operators to a log line that is now actually emitted by the code shipped in #2690.

— Authored by egg

@james-in-a-box

This comment has been minimized.

@james-in-a-box

Copy link
Copy Markdown
Contributor Author
egg is addressing review feedback...

@james-in-a-box

Copy link
Copy Markdown
Contributor Author

Review response

Thanks for the verification pass. The review verdict was COMMENTED with "No blocking issues" stated explicitly, and both non-blocking notes were pre-disposed by the reviewer as not requiring action. Per-item disposition:

  • Non-blocking note 1 — wording mismatch between "the hook falls back to /tmp" (prose) and "system temp" (in-code log message): disagree (reviewer explicitly stated "Not worth a follow-up; on Linux they're equivalent and the prose intentionally trades precision for operator readability." The mixed phrasing is intentional — the prose form is the operator-facing summary, the quoted form is verbatim from the log line so it grep's. Changing one to match the other would degrade one of those two purposes.)
  • Non-blocking note 2 — slice-BRC sentence reading as an addendum to a paragraph that previously ended with a "See [BRC-history file naming]" link: disagree (reviewer explicitly stated "Fine as-is; if a future edit reshuffles the paragraph, consider moving the forensics hint to its own line or a callout block." The note is forward-looking guidance for a future edit, not a request to act in this PR. Splitting the forensics hint into its own line now would either duplicate the "See [BRC-history file naming]" link or orphan it, both of which read worse than the current addendum form.)

No commits pushed; PR head remains at 10145687a0ffe82123958688f56489fd4a2d23f1.

— Authored by egg

@james-in-a-box

Copy link
Copy Markdown
Contributor Author

egg feedback addressed. View run logs

1 previous review(s) hidden.

@jwbron
jwbron merged commit 942b376 into main May 12, 2026
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant