Add verified cross-conversation continuity - #19
Conversation
|
@coderabbitai review |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe CLI adds deterministic checkpoints, expiring coordinator leases, scoped GBrain setup and health checks, updated onboarding and delivery contracts, expanded documentation, packaging support, and end-to-end coverage for continuity and safety behavior. ChangesContinuity and Project Memory
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 14
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@assets/project-template/.agent-stack/HANDOFF.md`:
- Around line 34-45: Update the memory decision flow in HANDOFF step 4 to ask
only when onboarding is pending or the configured memory/provider profile is
missing or has changed. Otherwise reuse the stored choice and continue without
prompting, while preserving the existing setup, guarded-plan, and doctor
confirmation steps when a new decision is required.
- Around line 46-47: Update the check-policy guidance near the instruction to
inspect project-manifest definitions so it does not reject every shell-backed
check: allow package scripts whose bodies invoke a shell, but reject command
arrays containing direct shell interpreters or destructive executables, and
inspect package-script bodies before approving them.
In `@assets/project-template/AGENTS.md`:
- Around line 9-12: Align both project-template pre-edit checklists with the
required source-of-truth inputs: in assets/project-template/AGENTS.md lines
9-12, add .agent-stack/core-policy.json; in
assets/project-template/.agent-stack/HANDOFF.md lines 8-9, add locked artifacts,
the current diff, and relevant tests/documentation. Ensure both checklists
require reading the project contract, policy, configuration, checkpoints, locked
artifacts, current diff, and relevant tests/documentation before editing.
In `@bin/ultimate-agent-stack.mjs`:
- Around line 3981-3988: Resolve the circular corrupt-checkpoint recovery in the
start flow around loadCheckpoint, acquireCoordinator, and commandCheckpoint.
Ensure a tampered checkpoint can be recovered without a coordinator token by
either acquiring the coordinator before checkpoint validation or updating the
invalid-checkpoint error guidance to explicitly instruct removing or moving the
checkpoint file first.
- Around line 3981-3988: Update bin/ultimate-agent-stack.mjs lines 3981-3988 in
the commandStart flow to acquire the coordinator lease before calling
loadCheckpoint, ensuring the recovery command can receive its token; update
lines 3931-3938 in commandStatus to wrap loadCheckpoint and readCoordinator in
try/catch and report failures as { error } in the checkpoint and coordinator
fields instead of throwing.
- Around line 3931-3938: Wrap the loadCheckpoint and readCoordinator calls in
the status command with try/catch handling, matching commandDoctor’s behavior
for StackError failures. Preserve successful values, but expose caught errors as
{ error } in the checkpoint and coordinator fields so status remains usable with
corrupt state.
- Around line 3300-3343: Update withCoordinatorMutex and its openMutex helper to
create and retain a unique owner marker containing the holder PID and creation
timestamp. During stale-lock reclamation, verify the marker still matches the
stale lock before removing it, then establish a new marker for the replacement
owner. In the finally block, read and compare the lock contents with this
invocation’s marker before unlinking, so cleanup cannot remove a successor’s
mutex.
- Around line 1961-1998: Update runGbrain to preserve the unmodified provider
stdout in a separate raw_stdout field while retaining redacted/truncated stdout
for reporting. Change parseProviderJson to parse raw_stdout, falling back
appropriately if unavailable, and continue using redacted stdout/stderr only for
surfaced detail and reports.
In `@docs/SKILL_STACK.md`:
- Around line 107-108: Update the GBrain description in the skill stack
documentation to say “project-scoped cross-conversation knowledge” and remove
the “cross-project” scope, while preserving the repository checkpoint fallback
wording.
In `@docs/TRUST.md`:
- Around line 122-126: Update the checkpoint mirroring statement in TRUST.md to
use security-neutral, repository-authoritative fallback terminology instead of
“fails open,” while preserving the documented behavior that the repository
remains the fallback authority.
In `@scripts/gbrain-project.mjs`:
- Around line 42-54: Update the launcher around the child spawned by the gbrain
serve startup flow to forward SIGTERM and SIGINT to the child process before
allowing the wrapper to exit. Ensure the signal handlers terminate the spawned
gbrain process and preserve the existing error and exit-status handling in the
child error and exit listeners.
- Around line 17-40: Replace the duplicated environment allowlist in the project
script with the shared exported names/helper from bin/ultimate-agent-stack.mjs.
Import and reuse that shared symbol when constructing environment, while
preserving the existing GBRAIN_HOME and NO_COLOR assignments.
In `@skills/run-autonomous-delivery/SKILL.md`:
- Around line 63-72: Update the delivery-loop guidance around the checkpoint and
handoff steps to require explicit human confirmation before any destructive,
irreversible, credential, financial, deployment, merge, or publication action.
State that “use the recommendation” does not authorize these high-impact
operations, and ensure the workflow pauses until confirmation is obtained.
In `@test/agent-stack.test.mjs`:
- Around line 192-233: Make installFakeGbrain’s generated extensionless
executable independent of the fixture root’s module type by using CommonJS
loading or adding a local module-type marker in its tool directory. Ensure the
helper works regardless of whether configureFixture runs first, while preserving
its existing command behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7e552198-3aa5-4872-8e3d-c3a4d4476d0c
📒 Files selected for processing (25)
.codex-plugin/plugin.jsonCHANGELOG.mdREADME.mdSTARTER_PROMPT.mdassets/project-template/.agent-stack/HANDOFF.mdassets/project-template/.agent-stack/gitignore.templateassets/project-template/AGENTS.mdbin/ultimate-agent-stack.mjsdocs/ADAPTERS.mddocs/ARCHITECTURE.mddocs/OPERATING_MANUAL.mddocs/SKILL_STACK.mddocs/TRUST.mdpackage.jsonscripts/gbrain-project.mjsskills/coordinate-parallel-delivery/SKILL.mdskills/coordinate-parallel-delivery/references/delegation-contract.mdskills/run-autonomous-delivery/SKILL.mdskills/setup-autonomous-project/SKILL.mdskills/setup-autonomous-project/references/setup-contract.mdskills/use-project-knowledge/SKILL.mdskills/use-project-knowledge/references/gbrain-provider.mdskills/use-project-knowledge/references/knowledge-contract.mdtest/agent-stack.test.mjstest/maintenance.test.mjs
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
bin/ultimate-agent-stack.mjs (2)
2135-2163: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMisattributed error when identity response is malformed but doctor is healthy.
healthyis false when eitheridentity.okis false,doctor.value?.status === "unhealthy", oridentity.value?.engineisn't a string. But theerrorfield only distinguishesidentity.oktrue/false — ifidentity.okis true yetengineis missing/wrong-typed (doctor otherwise healthy), the reported error is the misleading"gbrain doctor reported unhealthy"instead of describing the actual identity malformation.🐛 Proposed fix
- const healthy = - identity.ok && - doctor.value?.status !== "unhealthy" && - typeof identity.value?.engine === "string"; + const doctorUnhealthy = doctor.value?.status === "unhealthy"; + const identityMalformed = + identity.ok && typeof identity.value?.engine !== "string"; + const healthy = identity.ok && !doctorUnhealthy && !identityMalformed; return { ... ...(healthy ? {} - : { error: identity.ok ? "gbrain doctor reported unhealthy" : identity.error }), + : { + error: !identity.ok + ? identity.error + : doctorUnhealthy + ? "gbrain doctor reported unhealthy" + : "gbrain identity response is missing an engine identifier", + }),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/ultimate-agent-stack.mjs` around lines 2135 - 2163, Update the error selection in the gbrain health result around the healthy calculation so every unhealthy condition reports its actual cause: preserve the identity error when identity.ok is false, report an identity-malformation error when identity.ok is true but identity.value?.engine is not a string, and report the doctor-unhealthy error only when the doctor status is unhealthy.
3304-3390: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMutex race resolved; new unsafe-
throw-in-finallybug in the cleanup path.The owner-marker + compare-before-reclaim logic correctly resolves the prior "mutex cleanup can delete a lock owned by another process" concern. However, the cleanup
finallyblock (lines 3379-3389) rethrows any non-ENOENTerror fromreadFileSync/unlinkSync. Per JS semantics (and per Biome'snoUnsafeFinally), athrowinsidefinallydiscards whateveroperation()returned or threw — so a legitimate error fromoperation()(e.g. "No active Project Steward owns this checkout") would be silently replaced by an unrelated filesystem error if the mutex file happens to be unreadable/unlinkable at cleanup time.🐛 Proposed fix
openMutex(); try { return operation(); } finally { try { if (readFileSync(mutex, "utf8") === marker) { unlinkSync(mutex); } } catch (error) { if (error.code !== "ENOENT") { - throw error; + process.stderr.write( + `Warning: failed to release coordinator mutex: ${error.message}\n`, + ); } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/ultimate-agent-stack.mjs` around lines 3304 - 3390, Update the cleanup logic in withCoordinatorMutex so filesystem errors during the finally block never throw and replace the result or exception from operation(). Preserve marker comparison and ENOENT handling, but record or otherwise handle non-ENOENT read/unlink failures without propagating them from finally.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@bin/ultimate-agent-stack.mjs`:
- Around line 2135-2163: Update the error selection in the gbrain health result
around the healthy calculation so every unhealthy condition reports its actual
cause: preserve the identity error when identity.ok is false, report an
identity-malformation error when identity.ok is true but identity.value?.engine
is not a string, and report the doctor-unhealthy error only when the doctor
status is unhealthy.
- Around line 3304-3390: Update the cleanup logic in withCoordinatorMutex so
filesystem errors during the finally block never throw and replace the result or
exception from operation(). Preserve marker comparison and ENOENT handling, but
record or otherwise handle non-ENOENT read/unlink failures without propagating
them from finally.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ac8c1de3-1dcd-4f20-bacb-c55709b1dca2
📒 Files selected for processing (8)
assets/project-template/.agent-stack/HANDOFF.mdassets/project-template/AGENTS.mdbin/ultimate-agent-stack.mjsdocs/SKILL_STACK.mddocs/TRUST.mdscripts/gbrain-project.mjsskills/run-autonomous-delivery/SKILL.mdtest/agent-stack.test.mjs
|
Fixed both outside-diff findings in 72aaeb3. GBrain health now distinguishes provider failure, doctor-unhealthy state, and a malformed identity response; a new regression test covers the malformed case. Coordinator-mutex cleanup now reports non-ENOENT cleanup failures without throwing from finally or replacing the operation result/error. Validation passed: 41 focused CLI tests, all 78 release tests, package dry-run, packed smoke, 49-file markdownlint, and git diff check. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai resolve |
✅ Action performedComments resolved and changes approved. |
Refreshing CodeRabbit approval after its no-action exact-head review of 72aaeb3.
|
@coderabbitai resolve |
✅ Action performedComments resolved and changes approved. |
Summary
Verification
npm run release:check— 77 tests, package dry run, packed-install smoke, 79 intended filesquick_validate.pymarkdownlint-cli2@0.20.0— 49 tracked Markdown files, zero errorsgit diff --checkSafety notes
README 2.mdis not includedSummary by CodeRabbit
memory-setupandmemory-health, plus newcheckpointandcoordinatorcommands.serve.