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
|
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 (11)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughWhen ChangesDelegated child session guards
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to The child startup guards and parent behavior have targeted test coverage. No actionable issue is established that should block merging after normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change reduces repeated shared-file and history-store work in delegated children while preserving local bookkeeping and command safeguards. No introduced security issue was established, but complete write isolation and parent-before-child preparation were not demonstrated. 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 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 10 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
There was a problem hiding this comment.
Approving. Clean, well-structured foundation for the #1690 chain.
Key strengths observed:
- Clear isolation boundary: Extracting parent-only work into
startParentSessionensures children skip expensive shared-state operations (asset installation, legacy migration, native review status negotiation, repository preparation) without dropping local lifecycle resets. - Resource protection: Suppressing history capture, skill registry watchers, and upstream pretty indexing in children prevents file lock contention and background resource leaks in the shared working directory.
- Paired test controls: Every child guard in
tests/gentle-ai-child-guards.test.tsand companion suites includes a matching parent assertion, verifying that the guards do not pass vacuously.
Ready for merge as PR 1 of 4.
barbatdev
left a comment
There was a problem hiding this comment.
Clean foundation for the stack. The child/parent split does exactly what the title promises: delegated children keep their session-local resets and the dev-binary toast, while asset install, model override migration, review negotiation, repository preparation, prompt-history capture, pi-pretty indexing, the skill-registry watcher and the startup banner all stay parent-owned. The new child-guard tests cover the asymmetry in both directions (the child skips, the parent still does), including reminder/YOLO/sidebar re-arming for the child's own session. Focused suites pass (57/57 across the five touched files) and typecheck holds the 186-diagnostic baseline. One registration for #1771: the guard reads permissionEnvironment in gentle-ai.ts but process.env elsewhere; intentional per extension, but worth keeping consistent when the forwarding lands.
Summary
GENTLE_PI_AGENTS_CHILD=1, a child now skips: package asset install, legacy model migration and saved model config, repository preparation and review-status negotiation, review-permission revoke/refresh (extensions/gentle-ai.ts, moved intostartParentSession); repository preparation ontool_result; the skill registry startup (.atl/writes and an fs watcher); history capture; the upstream pi-pretty fallback (FFF indexing); and the startup banner.tests/gentle-ai-dev-binary-surfacing.test.ts).ctx.hasUIis true, sohasUIis not a child signal; the guards useGENTLE_PI_AGENTS_CHILD.This is PR 1 of 4. On its own it hardens regular gentle-pi children, which already load the package through
settings.json, and changes nothing for isolated children yet.Issue
Refs #1690
PR type
type:bug)Changes
220046116ae5b9c3gentle-ai.ts,skill-registry.ts,history/index.ts,pi-pretty.ts,startup-banner.ts, with child and parent-control tests.c3f3c158startParentSession, so code after it still runs in children; tests assert the child resets.e8aa21d42efe8919Test plan
All four branches were verified on top of
main(653dad90) withenv -u GENTLE_PI_AGENTS_CHILD(delegated shells export it).node scripts/run-test-suite.mjs): 0 fail.node scripts/check-types.mjs,build-runtime-modules --check,verify-package-files: pass.Heads-up:
tests/history-session-scan-extract.test.tsfails now and then on mtime resolution under load (#1514). It is unrelated to this chain and passes alone.Known follow-ups (non-blocking, from review)
tests/gentle-ai-dev-binary-surfacing.test.ts(upstream) setsGENTLE_PI_AGENTS_CHILDwithout restoring it.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)Review only the commits listed under Changes; earlier commits belong to the PRs below it in the chain.
Summary by CodeRabbit