Keep agent Sentry reporting live until shutdown - #3165
Conversation
Independent code review for PR #3165Score: 86/100 Reviewed range: Spec / blocker fitThe main blocker fix is directionally correct. BLOCK findingsNone. WATCH findings
Validation run
Note: an explicit |
Attach application-error cleanup to the agent service lifecycle so readiness no longer tears down reporting while the server keeps serving. Startup and rollback cleanup errors are isolated so the original fatal path still reaches the expected exit or rejection. Constraint: Published agent services return from startup after readiness while the process remains live. Rejected: Keep runAgentServiceMain success-path cleanup | It flushes and resets the reporter immediately after readiness. Confidence: high Scope-risk: narrow Directive: Keep application-error cleanup owned by service shutdown, and keep cleanup failures from replacing startup failures. Tested: deno test --allow-all src/agent/hosted/veryfront-cloud-agent-service.test.ts src/agent/service/runtime.test.ts; deno fmt --check touched files; deno lint touched files; deno check touched files; deno task typecheck; deno task typecheck:consumer Not-tested: Full deno task verify and binary e2e suite were not run; pre-commit verify:quick hit unrelated missing release-assets API docs.
3b06074 to
53ee22a
Compare
Re-review for PR #3165 at
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 53ee22af4f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Keep service module lifecycle cleanup independent from transport shutdown so application-error flush/reset still runs when Deno, Bun, or Node server shutdown fails. Constraint: Agent application-error cleanup is attached to service runtime stop, but transports may fail before runtime stop runs. Rejected: Leave stopRuntime sequenced after successful transport stop only | It skips lifecycle cleanup on server.shutdown, server.stop, or server.close failure. Confidence: high Scope-risk: narrow Directive: Preserve the first shutdown failure while still attempting module lifecycle cleanup exactly once. Tested: deno test --allow-all src/server/service-server.test.ts; deno test --allow-all src/agent/hosted/veryfront-cloud-agent-service.test.ts src/agent/service/runtime.test.ts; deno fmt --check touched files; deno lint touched files; deno check touched files; deno task typecheck Not-tested: Full deno task verify and binary e2e suite were not run; prior PR verification noted unrelated release-assets docs gaps.
Track shutdown failure presence separately from the failure value so undefined or null transport/runtime failures are still rethrown after lifecycle cleanup runs. Constraint: JavaScript allows throwing or rejecting nullish values, and service shutdown must preserve the first failure exactly. Rejected: Use the failure value itself as the sentinel | undefined and null are valid failure values and were treated as no failure. Confidence: high Scope-risk: narrow Directive: Keep shutdown cleanup independent from transport failures and preserve first-failure identity, including nullish failures. Tested: deno test --allow-all src/server/service-server.test.ts; deno test --allow-all src/agent/hosted/veryfront-cloud-agent-service.test.ts src/agent/service/runtime.test.ts; deno fmt --check touched files; deno check touched files; deno run --allow-read --allow-run scripts/lint/check-test-typecheck-baseline.ts; deno task lint; deno task typecheck Not-tested: Full deno task verify and binary e2e suite were not run.
Re-review at
|
Prepare the next stable framework version as a stacked release so the Agent Sentry lifecycle fix can publish after approval without reusing an existing npm version. Constraint: PR #3165 remains unmerged and this release branch must be based on agent/sentry-agent-lifecycle at 51324d9. Rejected: Base on main | it would bypass the pending lifecycle fix that this release is intended to publish. Confidence: high Scope-risk: narrow Directive: After PR #3165 merges, retarget this release PR to main before merging. Tested: npm latest/version-unused checks for veryfront and @veryfront/ext-observability-sentry; GitHub tag/release absence checks for v0.1.1176; jq deno.json version check and VERSION sync assertion; deno fmt --check deno.json src/utils/version-constant.ts; deno check src/utils/version-constant.ts; stable-release-requested.sh returns true against origin/main. Not-tested: Full release CI and package publishing remain for protected PR CI/release automation; local pre-commit verify:quick is blocked by existing release-assets docs/reference gap.
Prepare the next stable framework version on top of the merged Agent Sentry lifecycle fix without reusing an existing npm version. Constraint: PR #3165 merged into origin/main at a226ff4, so the release branch must now contain only the version bump above main. Rejected: Keep the old stacked lifecycle commits | they are already represented by the squashed merge on main. Confidence: high Scope-risk: narrow Directive: Keep this release PR limited to the two version files until release automation publishes 0.1.1176. Tested: jq deno.json version check and VERSION sync assertion; deno fmt --check deno.json src/utils/version-constant.ts; deno check src/utils/version-constant.ts; diff and merge-base validated against origin/main. Not-tested: Full release CI and package publishing remain for protected PR CI/release automation; local pre-commit verify:quick is blocked by existing release-assets docs/reference gap.
2 participants
Summary
Validation
deno test --allow-all src/agent/service/runtime.test.ts src/agent/hosted/veryfront-cloud-agent-service.test.tsdeno fmt --check src/agent/service/runtime.ts src/agent/service/runtime.test.ts src/agent/hosted/veryfront-cloud-agent-service.ts src/agent/hosted/veryfront-cloud-agent-service.test.tsdeno lint src/agent/service/runtime.ts src/agent/service/runtime.test.ts src/agent/hosted/veryfront-cloud-agent-service.ts src/agent/hosted/veryfront-cloud-agent-service.test.tsdeno check src/agent/hosted/veryfront-cloud-agent-service.ts src/agent/service/runtime.ts src/agent/hosted/veryfront-cloud-agent-service.test.ts src/agent/service/runtime.test.tsdeno task typecheckdeno task typecheck:consumerNotes
verify:quickwas attempted and failed on unrelated existing docs gaps for./release-assets: missing@exampleJSDoc and missingdocs/api-reference/veryfront/release-assets.md.npm --prefix storybook ciwas run once to satisfy the local consumer typecheck prerequisite.