Skip to content

fix(shared): tolerate missing git-info extension in child sessions - #340

Merged
tt-a1i merged 2 commits into
openpi-dev:mainfrom
liwen-aaa:fix/child-session-gitinfo-realpath
Sep 2, 2026
Merged

fix(shared): tolerate missing git-info extension in child sessions#340
tt-a1i merged 2 commits into
openpi-dev:mainfrom
liwen-aaa:fix/child-session-gitinfo-realpath

Conversation

@liwen-aaa

Copy link
Copy Markdown
Contributor

excludeOpenPiGitInfoExtension computed the git-info path with a bare realpathSync.native at module top level, which threw ENOENT and failed the whole child-session module when extensions/git-info/index.ts was absent. That module is the child-session core for both subagents/ and workflows/, so a single missing file took down every Direct/Workflow child with no degradation path.

Compute the path lazily inside the function via canonicalExistingPath (try/catch, returns undefined) and treat undefined as "git-info absent, nothing to exclude": a trimmed fork, partial install, or packaging drift now degrades to one benign extra extension instead of total outage.

Add a regression test that re-imports the module with git-info temporarily renamed away and asserts the core exports still resolve.

@liwen-aaa
liwen-aaa force-pushed the fix/child-session-gitinfo-realpath branch from 5a1c59c to 315cd17 Compare September 2, 2026 03:24

@tt-a1i tt-a1i left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

方向和价值都认可,但有两个边界需要在合并前收紧:

  1. 当前把权威 git-info 目标路径交给 canonicalExistingPath,而该函数会吞掉所有 realpath 错误。这里应只把 ENOENT 解释为扩展确实不存在;权限错误、循环链接或其他无法验证身份的错误应继续 fail closed,避免子会话在不可验证时保留 Git 轮询扩展。

  2. 回归测试会临时重命名仓库中的真实 extensions/git-info/index.ts。测试被中断时可能留下损坏工作区,也可能与并行测试产生文件系统竞态。请改为 mock 目标 realpath 调用,或在临时副本/fixture 中验证,不要变更被跟踪的源码文件。

当前 exact head 315cd17;生产修复方向正确,完成这两项后可重新 review。

excludeOpenPiGitInfoExtension computed the git-info path with a bare
realpathSync.native at module top level, which threw ENOENT and failed the
whole child-session module when extensions/git-info/index.ts was absent.
That module is the child-session core for both subagents/ and workflows/,
so a single missing file took down every Direct/Workflow child with no
degradation path.

Compute the path lazily inside the function via canonicalExistingPath
(try/catch, returns undefined) and treat undefined as "git-info absent,
nothing to exclude": a trimmed fork, partial install, or packaging drift
now degrades to one benign extra extension instead of total outage.

Add a regression test that re-imports the module with git-info temporarily
renamed away and asserts the core exports still resolve.
@liwen-aaa
liwen-aaa force-pushed the fix/child-session-gitinfo-realpath branch from 315cd17 to c755bd1 Compare September 2, 2026 05:44

@tt-a1i tt-a1i left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Re-reviewed exact head c755bd1. The prior blockers are resolved: only ENOENT degrades, non-ENOENT path failures remain fail-closed, and the regression test no longer renames tracked source. Focused checks and current CI are green; repeated unrelated timing failures passed on isolated rerun.

@tt-a1i tt-a1i left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Approved exact head f921a57 after updating from protected main. The merge commit contains the previously reviewed c755bd1 change plus main commit 46ae881; the PR diff remains limited to child-session.ts and its regression test. All required CI checks are green.

@tt-a1i
tt-a1i merged commit 72fbba5 into openpi-dev:main Sep 2, 2026
4 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.

2 participants