Skip to content

fix: make AutoPG lifecycle readiness-aware - #143

Open
filipexyz wants to merge 1 commit into
mainfrom
agent/autopg-lifecycle-readiness
Open

filipexyz wants to merge 1 commit into
mainfrom
agent/autopg-lifecycle-readiness

Conversation

@filipexyz

@filipexyz filipexyz commented Aug 7, 2026

Copy link
Copy Markdown

Summary

  • restart the canonical autopg-server PM2 process instead of the removed legacy entry
  • make install and restart succeed only after the live postmaster runtime matches the configured port and PM2 process
  • start an existing stopped PM2 entry instead of reporting it as already installed
  • expose readiness and supervisor state separately so status cannot report a stopped service as healthy
  • fail clearly instead of spawning an unmanaged legacy daemon

Validation

  • bun test
  • bun run lint
  • bun run deadcode

Summary by CodeRabbit

  • New Features

    • Added service readiness monitoring that combines supervisor and PostgreSQL runtime status.
    • Install and restart operations now wait until the service is ready before reporting success.
    • Status output now includes readiness and supervisor details, with clearer degraded or failure explanations.
  • Bug Fixes

    • Improved handling of stopped, stale, or mismatched service processes.
    • Standardized restart failures and timeout messages with actionable guidance.
  • Documentation

    • Updated autopg restart documentation to reflect readiness-based success criteria.

@coderabbitai

coderabbitai Bot commented Aug 7, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Changes

Service readiness lifecycle

Layer / File(s) Summary
Service-state evaluation
src/lib/service-state.cjs, tests/lib/service-state.test.js
Adds PM2 and runtime inspection, readiness classification, diagnostic formatting, polling, and coverage for ready and degraded states.
Install and status readiness integration
src/cli-install.cjs, tests/cli-install.test.js
Install paths wait for readiness. Status output reports readiness, computed status, runtime state, and supervisor status.
PM2-only restart and supervisor naming
src/cli-restart.cjs, src/commands/doctor.js, tests/cli/restart.test.js, README.md
Restart requires PM2 registration and readiness. Doctor uses the shared process name. Documentation reflects the readiness requirement.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant CLI as autopg restart
  participant PM2
  participant ServiceState as service-state helpers
  participant Runtime as runtime.json
  CLI->>PM2: verify autopg-server registration
  CLI->>PM2: restart autopg-server
  CLI->>ServiceState: waitForServiceReadiness()
  ServiceState->>PM2: inspect supervisor state
  ServiceState->>Runtime: inspect runtime metadata
  ServiceState-->>CLI: ready or formatted failure
Loading

Possibly related PRs

Suggested reviewers: namastex888

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: making AutoPG lifecycle operations readiness-aware.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch agent/autopg-lifecycle-readiness

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 5

🧹 Nitpick comments (4)
tests/lib/service-state.test.js (1)

8-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add coverage for the stopped and failed status branches.

The tests cover ready and degraded. They do not cover the classification at src/lib/service-state.cjs lines 105-106: supervisorStatus of stopped or missing maps to stopped, and errored maps to failed. The install and status paths now report these values to operators. The non-pm2 supervisor branch (supervisor: 'external') is also untested; it makes pm2Ready and runtimeOwnedBySupervisor unconditionally true.

🧪 Proposed additional cases
+  test('classifies a stopped pm2 entry as stopped', () => {
+    const state = evaluateServiceState({
+      supervisor: 'pm2',
+      supervisorStatus: 'stopped',
+      supervisorPid: null,
+      configuredPort: 5432,
+      runtime: null,
+      runtimeLive: false,
+    });
+
+    expect(state.ready).toBe(false);
+    expect(state.status).toBe('stopped');
+  });
+
+  test('classifies an errored pm2 entry as failed', () => {
+    const state = evaluateServiceState({
+      supervisor: 'pm2',
+      supervisorStatus: 'errored',
+      supervisorPid: null,
+      configuredPort: 5432,
+      runtime: null,
+      runtimeLive: false,
+    });
+
+    expect(state.status).toBe('failed');
+  });
🤖 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 `@tests/lib/service-state.test.js` around lines 8 - 67, Add tests in the
evaluateServiceState suite for supervisorStatus values stopped and missing
mapping to stopped, and errored mapping to failed, asserting readiness, status,
and relevant reasons as appropriate. Also cover the supervisor: 'external'
branch, verifying pm2Ready and runtimeOwnedBySupervisor are treated as true and
the resulting classification is correct.
src/cli-restart.cjs (1)

14-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Reuse the shared pm2GetProcess instead of duplicating it.

Lines 20-34 duplicate pm2GetProcess from src/lib/service-state.cjs lines 48-62 exactly. This file already imports from that module. The duplicate creates two places to change the PM2 probe timeout or the parsing rules. Export the helper from the module's public surface, or read it from _internals.

🤖 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 `@src/cli-restart.cjs` around lines 14 - 34, Remove the local pm2GetProcess
implementation and reuse the shared helper from service-state.cjs. Export
pm2GetProcess through that module’s public API, or obtain it via the existing
_internals surface, then update cli-restart.cjs to reference the shared symbol
while preserving its current process lookup behavior.
tests/cli-install.test.js (1)

615-631: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add coverage for the install readiness-failure branch.

This test covers the degraded status report. No test covers the new failure branch at src/cli-install.cjs lines 1034-1039, where a fresh install aborts because the service never becomes ready. That branch waits 30 s by default and produces the operator-facing diagnostic. A test that suppresses the stub's refreshRuntime before install would cover it. Pass a short readiness timeout, or keep the default and accept the runtime cost.

🤖 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 `@tests/cli-install.test.js` around lines 615 - 631, Add a test covering the
fresh-install readiness failure branch in the install flow, where the service
never becomes ready and the operator-facing diagnostic is emitted. Suppress the
stub’s refreshRuntime before invoking install, and configure a short readiness
timeout so the test remains fast; assert that installation aborts with the
expected failure result and diagnostic.
src/commands/doctor.js (1)

263-266: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

The message strings still hardcode the process name.

Line 263 now uses PM2_PROCESS_NAME. Lines 265-266 still embed the literal autopg-server in the operator messages. If the constant changes, the probe and the reported name diverge. Interpolate the constant in the messages.

♻️ Proposed change
       const r = pm2EntryOnline(PM2_PROCESS_NAME);
       return r.ok
-        ? check('supervisor_liveness', 'pm2 autopg-server entry online', SEVERITY.PASS)
-        : check('supervisor_liveness', 'pm2 autopg-server entry not online', SEVERITY.FAIL, r.reason, 'run `pgserve install` to (re-)register pm2 entry');
+        ? check('supervisor_liveness', `pm2 ${PM2_PROCESS_NAME} entry online`, SEVERITY.PASS)
+        : check('supervisor_liveness', `pm2 ${PM2_PROCESS_NAME} entry not online`, SEVERITY.FAIL, r.reason, 'run `pgserve install` to (re-)register pm2 entry');
🤖 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 `@src/commands/doctor.js` around lines 263 - 266, Update the success and
failure messages in the supervisor_liveness check to interpolate
PM2_PROCESS_NAME instead of hardcoding “autopg-server,” keeping the existing
check behavior and remediation text unchanged.
🤖 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 `@README.md`:
- Line 140: Update the neighbouring autopg status documentation entry to
describe cmdStatus’s computed status and ready fields as readiness
classification derived from PM2 state and the postmaster runtime record, and
mention supervisorStatus for the raw PM2 value instead of the outdated on-disk
config snapshot wording.

In `@src/cli-install.cjs`:
- Around line 899-910: Update the pre-flight condition around
assertPortAvailable so it skips only when an existing PM2 entry will be
retained; ensure --redeploy still performs the port check because its existing
entry is later removed and replaced.
- Around line 1008-1016: Detect whether the requested port differs from the
existing supervisor configuration before calling writeSupervisorRecord in the
installation flow. When the port changes and the live process is intentionally
retained, fail immediately with guidance to rerun using --redeploy; preserve
idempotent success for unchanged ports and ensure the new port is not persisted
to admin.json before validation.

In `@tests/cli-install.test.js`:
- Around line 82-96: Update the start handling around the registered and online
sentinel writes so they occur only when the stub is in success mode, while
preserving the existing failure exit behavior. Ensure failed pm2 start attempts
do not write service state or trigger refreshRuntime, and successful starts
retain the current registration flow.

In `@tests/lib/service-state.test.js`:
- Around line 1-6: Update tests/lib/service-state.test.js:1-6 and
tests/cli/restart.test.js:9-18 to support CommonJS loading in strict ESM tests
by creating a local require with node:module’s createRequire before each require
call. In tests/cli/restart.test.js, also replace __dirname with a path derived
from import.meta.url while preserving the existing path behavior.

---

Nitpick comments:
In `@src/cli-restart.cjs`:
- Around line 14-34: Remove the local pm2GetProcess implementation and reuse the
shared helper from service-state.cjs. Export pm2GetProcess through that module’s
public API, or obtain it via the existing _internals surface, then update
cli-restart.cjs to reference the shared symbol while preserving its current
process lookup behavior.

In `@src/commands/doctor.js`:
- Around line 263-266: Update the success and failure messages in the
supervisor_liveness check to interpolate PM2_PROCESS_NAME instead of hardcoding
“autopg-server,” keeping the existing check behavior and remediation text
unchanged.

In `@tests/cli-install.test.js`:
- Around line 615-631: Add a test covering the fresh-install readiness failure
branch in the install flow, where the service never becomes ready and the
operator-facing diagnostic is emitted. Suppress the stub’s refreshRuntime before
invoking install, and configure a short readiness timeout so the test remains
fast; assert that installation aborts with the expected failure result and
diagnostic.

In `@tests/lib/service-state.test.js`:
- Around line 8-67: Add tests in the evaluateServiceState suite for
supervisorStatus values stopped and missing mapping to stopped, and errored
mapping to failed, asserting readiness, status, and relevant reasons as
appropriate. Also cover the supervisor: 'external' branch, verifying pm2Ready
and runtimeOwnedBySupervisor are treated as true and the resulting
classification is correct.
🪄 Autofix

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: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 8563b9d7-de90-4a3c-aecf-87e2b59e413d

📥 Commits

Reviewing files that changed from the base of the PR and between 7e8d508 and 0274425.

📒 Files selected for processing (8)
  • README.md
  • src/cli-install.cjs
  • src/cli-restart.cjs
  • src/commands/doctor.js
  • src/lib/service-state.cjs
  • tests/cli-install.test.js
  • tests/cli/restart.test.js
  • tests/lib/service-state.test.js

Comment thread README.md
autopg url | autopg port # canonical connection string / port
autopg config <list|get|set|edit|path|init> # manage ~/.autopg/settings.json
autopg restart # pm2-aware: pm2 restart pgserve, else SIGTERM+respawn
autopg restart # restart autopg-server; succeeds only when PostgreSQL is ready

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

The restart description is accurate. Update the status description in the same block.

Line 140 matches the new behavior in src/cli-restart.cjs. The neighbouring autopg status description still reads "pm2 + on-disk config snapshot". cmdStatus now reports a computed status and ready field derived from PM2 state and the postmaster runtime record, and it moved the PM2 value to supervisorStatus. Operators reading this table will not learn that the reported status is a readiness classification.

📝 Proposed change
-autopg status                          # pm2 + on-disk config snapshot
+autopg status                          # readiness + supervisor state + on-disk config snapshot
🤖 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 `@README.md` at line 140, Update the neighbouring autopg status documentation
entry to describe cmdStatus’s computed status and ready fields as readiness
classification derived from PM2 state and the postmaster runtime record, and
mention supervisorStatus for the raw PM2 value instead of the outdated on-disk
config snapshot wording.

Comment thread src/cli-install.cjs
Comment on lines +899 to +910
const noUi = args.includes('--no-ui');
const withUi = args.includes('--with-ui');
const redeploy = args.includes('--redeploy');
const existingBeforeInstall = pm2GetProcess(PM2_PROCESS_NAME);

// B3 (v2.6.1): pre-flight bind-test the chosen port BEFORE creating
// pm2 entries / admin.json / data dir. Without this, an operator on
// a host where 5432 is already occupied gets pm2 reporting `online`
// while the postmaster crashes silently — divergence between
// supervisor state and data-plane state. Fail fast with a clear hint.
try {
await assertPortAvailable(port);
if (!withUi && !existingBeforeInstall) await assertPortAvailable(port);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

--redeploy now skips the port pre-flight.

Line 910 skips assertPortAvailable whenever a PM2 entry already exists. With --redeploy, that entry is deleted at lines 985-992 and a fresh postmaster is started. The redeploy path is therefore a fresh install, but it never bind-tests the port. If a foreign process holds the port, the operator loses the fast EADDRINUSE diagnostic that the B3 comment describes, and instead waits the full 30 s readiness timeout for a generic "did not become ready" message.

Skip the pre-flight only when the existing entry is kept.

🐛 Proposed fix
   try {
-    if (!withUi && !existingBeforeInstall) await assertPortAvailable(port);
+    if (!withUi && (redeploy || !existingBeforeInstall)) await assertPortAvailable(port);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const noUi = args.includes('--no-ui');
const withUi = args.includes('--with-ui');
const redeploy = args.includes('--redeploy');
const existingBeforeInstall = pm2GetProcess(PM2_PROCESS_NAME);
// B3 (v2.6.1): pre-flight bind-test the chosen port BEFORE creating
// pm2 entries / admin.json / data dir. Without this, an operator on
// a host where 5432 is already occupied gets pm2 reporting `online`
// while the postmaster crashes silently — divergence between
// supervisor state and data-plane state. Fail fast with a clear hint.
try {
await assertPortAvailable(port);
if (!withUi && !existingBeforeInstall) await assertPortAvailable(port);
const noUi = args.includes('--no-ui');
const withUi = args.includes('--with-ui');
const redeploy = args.includes('--redeploy');
const existingBeforeInstall = pm2GetProcess(PM2_PROCESS_NAME);
// B3 (v2.6.1): pre-flight bind-test the chosen port BEFORE creating
// pm2 entries / admin.json / data dir. Without this, an operator on
// a host where 5432 is already occupied gets pm2 reporting `online`
// while the postmaster crashes silently — divergence between
// supervisor state and data-plane state. Fail fast with a clear hint.
try {
if (!withUi && (redeploy || !existingBeforeInstall)) await assertPortAvailable(port);
🤖 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 `@src/cli-install.cjs` around lines 899 - 910, Update the pre-flight condition
around assertPortAvailable so it skips only when an existing PM2 entry will be
retained; ensure --redeploy still performs the port check because its existing
entry is later removed and replaced.

Comment thread src/cli-install.cjs
Comment on lines 1008 to +1016
writeConfig({ port, dataDir, registeredAt: readConfig()?.registeredAt ?? new Date().toISOString() });
writeSupervisorRecord(adminJson, { supervisor: 'pm2', socketDir, port });
const state = await waitForServiceReadiness();
if (!state.ready) {
fail(
`pm2 process "${PM2_PROCESS_NAME}" did not become ready: ${formatServiceState(state)}. `
+ `Logs: ${getLogsDir()}/${PM2_PROCESS_NAME}-error.log`,
);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Re-running install with a different --port now fails and leaves a wrong admin.json.

Line 1009 writes the newly parsed port into the supervisor record. The comment at lines 1005-1007 states that the live process is intentionally not torn down, so it keeps serving the old port. waitForServiceReadiness reads configuredPort from admin.json and compares it to runtime.port (see src/lib/service-state.cjs lines 77-79 and 137-141). The two values now disagree, so readiness never succeeds and line 1012 aborts with runtime port X does not match configured port Y.

Two effects follow. The documented idempotent no-op success becomes a 30 s hang plus a failure. admin.json is already persisted with a port that nothing listens on, so autopg url and downstream discovery point at a dead port.

Detect the port change before writing the record and tell the operator to use --redeploy.

🐛 Proposed fix
+    // A live entry keeps its original port. Refusing here avoids
+    // recording a port that nothing listens on.
+    const livePort = readConfig()?.port ?? null;
+    if (Number.isInteger(livePort) && livePort !== port) {
+      fail(`pm2 process "${PM2_PROCESS_NAME}" is already running on port ${livePort}. Pass --redeploy to move it to port ${port}.`);
+    }
     writeConfig({ port, dataDir, registeredAt: readConfig()?.registeredAt ?? new Date().toISOString() });
     writeSupervisorRecord(adminJson, { supervisor: 'pm2', socketDir, port });
     const state = await waitForServiceReadiness();
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
writeConfig({ port, dataDir, registeredAt: readConfig()?.registeredAt ?? new Date().toISOString() });
writeSupervisorRecord(adminJson, { supervisor: 'pm2', socketDir, port });
const state = await waitForServiceReadiness();
if (!state.ready) {
fail(
`pm2 process "${PM2_PROCESS_NAME}" did not become ready: ${formatServiceState(state)}. `
+ `Logs: ${getLogsDir()}/${PM2_PROCESS_NAME}-error.log`,
);
}
// A live entry keeps its original port. Refusing here avoids
// recording a port that nothing listens on.
const livePort = readConfig()?.port ?? null;
if (Number.isInteger(livePort) && livePort !== port) {
fail(`pm2 process "${PM2_PROCESS_NAME}" is already running on port ${livePort}. Pass --redeploy to move it to port ${port}.`);
}
writeConfig({ port, dataDir, registeredAt: readConfig()?.registeredAt ?? new Date().toISOString() });
writeSupervisorRecord(adminJson, { supervisor: 'pm2', socketDir, port });
const state = await waitForServiceReadiness();
if (!state.ready) {
fail(
`pm2 process "${PM2_PROCESS_NAME}" did not become ready: ${formatServiceState(state)}. `
`Logs: ${getLogsDir()}/${PM2_PROCESS_NAME}-error.log`,
);
}
🤖 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 `@src/cli-install.cjs` around lines 1008 - 1016, Detect whether the requested
port differs from the existing supervisor configuration before calling
writeSupervisorRecord in the installation flow. When the port changes and the
live process is intentionally retained, fail immediately with guidance to rerun
using --redeploy; preserve idempotent success for unchanged ports and ensure the
new port is not persisted to admin.json before validation.

Comment thread tests/cli-install.test.js
Comment on lines 82 to 96
if (args[0] === 'start') {
fs.writeFileSync(${JSON.stringify(path.join(dir, 'registered'))}, '');
fs.writeFileSync(${JSON.stringify(statusFile)}, 'online');
const nameIndex = args.indexOf('--name');
if (nameIndex >= 0 && args[nameIndex + 1] === 'autopg-server') {
const socketIndex = args.lastIndexOf('--socket-dir');
const portIndex = args.lastIndexOf('--port');
fs.writeFileSync(serviceStatePath, JSON.stringify({
socketDir: args[socketIndex + 1],
port: Number(args[portIndex + 1])
}));
refreshRuntime();
}
process.exit(${exitCode});
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

The stub marks the process registered and online even in failure mode.

Lines 83-84 write the registered sentinel and the online status before line 95 exits with exitCode, which is 1 in failure mode. A failed pm2 start therefore leaves the stub reporting a healthy autopg-server on the next jlist. Any test that asserts on install failure and then inspects state observes a process that was never started.

Write the sentinel and the status only on the success path.

🧪 Proposed fix
 if (args[0] === 'start') {
+  if (${exitCode} !== 0) { process.exit(${exitCode}); }
   fs.writeFileSync(${JSON.stringify(path.join(dir, 'registered'))}, '');
   fs.writeFileSync(${JSON.stringify(statusFile)}, 'online');
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (args[0] === 'start') {
fs.writeFileSync(${JSON.stringify(path.join(dir, 'registered'))}, '');
fs.writeFileSync(${JSON.stringify(statusFile)}, 'online');
const nameIndex = args.indexOf('--name');
if (nameIndex >= 0 && args[nameIndex + 1] === 'autopg-server') {
const socketIndex = args.lastIndexOf('--socket-dir');
const portIndex = args.lastIndexOf('--port');
fs.writeFileSync(serviceStatePath, JSON.stringify({
socketDir: args[socketIndex + 1],
port: Number(args[portIndex + 1])
}));
refreshRuntime();
}
process.exit(${exitCode});
}
if (args[0] === 'start') {
if (${exitCode} !== 0) { process.exit(${exitCode}); }
fs.writeFileSync(${JSON.stringify(path.join(dir, 'registered'))}, '');
fs.writeFileSync(${JSON.stringify(statusFile)}, 'online');
const nameIndex = args.indexOf('--name');
if (nameIndex >= 0 && args[nameIndex + 1] === 'autopg-server') {
const socketIndex = args.lastIndexOf('--socket-dir');
const portIndex = args.lastIndexOf('--port');
fs.writeFileSync(serviceStatePath, JSON.stringify({
socketDir: args[socketIndex + 1],
port: Number(args[portIndex + 1])
}));
refreshRuntime();
}
process.exit(${exitCode});
}
🤖 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 `@tests/cli-install.test.js` around lines 82 - 96, Update the start handling
around the registered and online sentinel writes so they occur only when the
stub is in success mode, while preserving the existing failure exit behavior.
Ensure failed pm2 start attempts do not write service state or trigger
refreshRuntime, and successful starts retain the current registration flow.

Comment on lines +1 to +6
import { describe, expect, test } from 'bun:test';

const {
evaluateServiceState,
waitForServiceReadiness,
} = require('../../src/lib/service-state.cjs');

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

ESM test files use CommonJS require without ESLint support. Both new test files import from bun:test with ESM syntax and then load repository modules with require. ESLint reports 'require' is not defined in each file. The PR lists bun run lint as a validation step, so the lint run fails. Fix this once by declaring the CommonJS globals for the test glob in the ESLint config, or per file with createRequire.

  • tests/lib/service-state.test.js#L1-L6: add import { createRequire } from 'node:module'; and const require = createRequire(import.meta.url); before the require call at line 6.
  • tests/cli/restart.test.js#L9-L18: apply the same createRequire declaration for lines 16-17, and replace __dirname at line 12 with a path derived from import.meta.url if the file is strict ESM.
🧰 Tools
🪛 ESLint

[error] 6-6: 'require' is not defined.

(no-undef)

📍 Affects 2 files
  • tests/lib/service-state.test.js#L1-L6 (this comment)
  • tests/cli/restart.test.js#L9-L18
🤖 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 `@tests/lib/service-state.test.js` around lines 1 - 6, Update
tests/lib/service-state.test.js:1-6 and tests/cli/restart.test.js:9-18 to
support CommonJS loading in strict ESM tests by creating a local require with
node:module’s createRequire before each require call. In
tests/cli/restart.test.js, also replace __dirname with a path derived from
import.meta.url while preserving the existing path behavior.

Source: Linters/SAST tools

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.

1 participant