Repository navigation
Conversation
…nd audit child hooks
Guard gentle-ai session_start side effects, repository preparation on tool_result, skill-registry startup, history capture, the pi-pretty fallback and the startup banner when GENTLE_PI_AGENTS_CHILD=1, so a child that loads the full package does not rewrite shared state. Refs Gentleman-Programming#1690
…nding follow-ups
Move the parent-owned session_start steps into startParentSession so a delegated child skips only that work, and assert that the child-local resets (elapsed-timing ledger, reminder re-arm) still run. Refs Gentleman-Programming#1690
Await the tool_execution_start handlers before counting ledger entries, and assert that a restarted child clears its YOLO indicator and re-arms the review sidebar for its own session. Refs Gentleman-Programming#1690
Upstream expects a child session to surface the dev-binary warning fallback. Extract the notice from the parent-only startup work so children still run it while skipping the rest. Refs Gentleman-Programming#1690
buildPiInvocation now sets GENTLE_SHELL_CHILD_PACKAGE_INJECTION (JSON v1) with the exact -e set it injects: the full takeover set with noExtensions, or the package root when settings declare no gentle-pi. Declared and pi-subcommand launches drop any inherited value. The wire format lives in lib/child-package-injection.ts, built into the runtime. Refs Gentleman-Programming#1690
… cwd buildPiInvocation takes the cwd the spawn uses instead of reading process.cwd(), and dedupes the exported signal after making paths absolute, so one file spelled two ways appears once. The -e argv is unchanged. Refs Gentleman-Programming#1690
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (21)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review. 📝 WalkthroughWalkthroughThe launcher now provides child-package injection metadata for selected invocation cases. Extensions also use the child-session flag to skip selected parent-session initialization, history capture, banner setup, and repository preparation. ChangesChild Session Behavior
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Feature Suggested reviewers: Merge Risk: ⚪ Minimal · up to The package-injection signal and child-session guards are ready to merge after normal checks. Runner-side forwarding remains a documented later task, not part of this change. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The launcher exports paths it already selected for the parent, and the child-session guards preserve mutation tracking and command restrictions while avoiding parent-owned work. No introduced security defect was established. Risk remains nonminimal because the actual delegated-child consumer and its execution controls could not be verified. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 42.86% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 21 functions across 20 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches🧪 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 |
carlosmoradev
left a comment
carlosmoradev
left a comment
There was a problem hiding this comment.
Approving. The contract in lib/child-package-injection.ts is solid.
Key highlights:
- Explicit wire contract: Exporting
{version: 1, noExtensions, extensionPaths}via an explicit environment signal avoids guessing or fragile argv inspection downstream in the runner. - Path normalization: Resolving relative paths against an explicit
cwdbefore deduplicating ensures that child processes in subdirectories resolve the correct package root without path drift. - Safe degradation: The parser handles malformed or unknown-version signals fail-closed without throwing.
Solid step 2 of 4.
barbatdev
left a comment
barbatdev
left a comment
There was a problem hiding this comment.
Approved. The signal contract is sound: all four branches set or explicitly delete the env (so an inherited value never leaks into a subcommand or declared home), passthrough -e stays out of the signal while argv keeps it, paths are absolutized against the spawn's actual cwd and deduped after, and the parser never throws while gating on version === 1. The generated runtime copy is mechanically reconciled and verify-package-files holds.
Focused suites pass on my side too (145/145 across the injection, bin and child-guard files, typecheck at the 186-diagnostic baseline).
Non-blocking notes, mostly for #1772 since it becomes the first consumer:
- The parser rejects
extensionPaths: [], so a semantically validnoExtensions: truewith zero paths cannot round-trip today. Unreachable from the launcher, but the first consumer will have to respect it. - The encoder does not validate, so it can emit values the parser rejects (a relative path, an empty list). No test pins that asymmetry.
- Version mismatch degrades silently to
undefined. Safe, but worth one line of policy somewhere: bump means new key, old keys stay parseable for one cycle, or similar. - Bin-level e2e asserts the signal only for the no-declaration branch; the takeover assertion would pin the branch this stack actually exercises in production.
3 participants
Summary
pi -e <packageRoot>(isolated home, or a takeover), but never says so, so the subagent runner cannot give children the same package.buildPiInvocationnow exportsGENTLE_SHELL_CHILD_PACKAGE_INJECTION, JSON{version: 1, noExtensions, extensionPaths}:-eset, withnoExtensions: true;settings.json:[packageRoot];-e(the managed herdr extension, or one the user typed) is not part of the signal.cwd(the bin passes its own, which the spawn uses), and deduped after that.lib/child-package-injection.ts(encoder, a parser that never throws, the child argv helper) and is generated intoruntime/.This is PR 2 of 4. Nothing consumes the signal yet; #1772 does.
Issue
Refs #1690
PR type
type:feature)Changes
ac5ea634lib/child-package-injection.ts, the signal inbuildPiInvocation, the generated runtime, registration inbuild-runtime-modulesandverify-package-files, tests at lib and bin level.f493b3afcwdinstead ofprocess.cwd(); dedupe the signal after making paths absolute.1030d68dcwdin the upstream bin fixture.Test plan
All four branches were verified on top of
main(653dad90) withenv -u GENTLE_PI_AGENTS_CHILD(delegated shells export it).check-types,build-runtime-modules --check,verify-package-files: pass.Known follow-ups (non-blocking, from review)
cwdand the spawn sets none.packageRoot.Chain Context
main(each PR is opened againstmain; until its predecessors merge, its diff also shows their commits, and I rebase it as they land)runtime/*.mjscopies and about 230 are tests.Review only the commits listed under Changes; earlier commits belong to the PRs below it in the chain.
Summary by CodeRabbit