Skip to content

PLN-352: Characterize telemetry-loop-integration.test.ts drift (Class C — not stale)#124

Merged
peterulsteen merged 1 commit into
symphony/prd-220from
symphony/pln-352
Apr 27, 2026
Merged

PLN-352: Characterize telemetry-loop-integration.test.ts drift (Class C — not stale)#124
peterulsteen merged 1 commit into
symphony/prd-220from
symphony/pln-352

Conversation

@peterulsteen
Copy link
Copy Markdown
Contributor

@peterulsteen peterulsteen commented Apr 23, 2026

Drift Characterization: telemetry-loop-integration.test.ts

Commit SHA at time of analysis: 6c8ac24

Classification: Class C — Not Stale

apps/desktop/test/telemetry-loop-integration.test.ts is not stale. All 6 tests pass against current production code without modification. The test correctly exercises the current production spawn and telemetry emission paths.

Correction of PRD Line Number Citations

The PRD cited symphony-loop.ts lines 1733 and 3697–3830 as "production spawn sites." These citations are inaccurate:

  • Line 1733: Prompt construction for the claude CLI — not a spawn site.
  • Lines 3697–3830: cloneRepoViaGh() / repo resolution logic — not spawn sites.

Actual Production Spawn Sites (symphony-loop.ts)

All spawn calls use Node.js native child_process.spawn() with stdio arrays. No PTY API is involved anywhere in the codebase.

Line Command stdio
1836 LLM commit assistant (claudeBinary) "pipe" (stdout/stderr streamed)
4349 DECOMPOSE pipeline (buildClaudePipeline) ["ignore", logFd, logFd]
4374 EVALUATE_PRD pipeline (buildClaudePipeline) ["ignore", logFd, logFd]
4407 EVALUATE_PLAN / EVALUATE_CODE pipeline (buildClaudePipeline) ["ignore", logFd, logFd]
4446 REQUEST_CHANGES pipeline (buildClaudePipeline) ["ignore", logFd, logFd]
4467 GENERATE_PRD pipeline (buildClaudePipeline) ["ignore", logFd, logFd]
4500 PLAN / EXECUTE — run-loop.sh script ["ignore", logFd, logFd]

All pipeline spawn calls are preceded by getResolvedClaudePath() (from apps/desktop/src/server/shell-path.ts) which the test exercises via setShellPathForTest() + process.env.PATH manipulation.

Actual Telemetry Emission Sites (symphony-loop.ts)

Line Category Trigger
2923 job.failed Child process exits with non-zero code
3339 job.completed Child process exits 0 (JobStore path)
3409 job.completed Child process exits 0 (legacy path)
4162 preflight.binary_not_found getResolvedClaudePath() returns a path that does not exist
4253 preflight.spawn_failed openSync(logFile, "a") throws (e.g. EISDIR)
4517 preflight.spawn_failed child.on("error") fires after spawn (e.g. ENOENT)

Why the Test Is Not Stale (Class C Rationale)

  1. Binary resolution path: Tests 1–5 place a fake claude binary on PATH and call setShellPathForTest(). This correctly exercises getResolvedClaudePath() (the "path" resolution strategy), which is the same path production uses when no override is configured.

  2. Spawn path coverage: Tests 1, 2, and 5 use the DECOMPOSE command which hits the spawn at line 4349. Test 4 (preflight.spawn_failed) uses the PLAN command which reaches the log-file open at line 4253 before spawning. Both paths remain in production.

  3. Telemetry category alignment: The test targets all four current telemetry categories (job.failed, job.completed, preflight.binary_not_found, preflight.spawn_failed) which are the exact categories emitted by the current production code. No deprecated or renamed categories are asserted.

  4. No PTY references: Zero matches for spawnPtySession, spawnPty, pty.spawn, or node-pty in apps/desktop/src/ or apps/desktop/test/. The original framing of a "PTY migration" does not correspond to anything in the current codebase. All spawn calls use native child_process.spawn() with stdio arrays.

  5. Test pass evidence (2026-04-23, commit 6c8ac24):

    • job.failed emitted with correct category/trace/diagnostics on process exit non-zero — PASS (212ms)
    • job.completed emitted with correct category/trace on process exit 0 — PASS (158ms)
    • preflight.binary_not_found emitted when claude is absent from PATH — PASS (12ms)
    • preflight.spawn_failed emitted when log file open fails (EISDIR) — PASS (15ms)
    • commandId and operationId from request headers appear in trace context — PASS (138ms)
    • Observability truncates logTail to TELEMETRY_MAX_FIELD_BYTES via TelemetryService — PASS (<1ms)
    • Total: 6/6 pass, 0 fail, 1622ms

Remediation Approach

Per PRD behavioral details for Class C: "drop the 'stale' label, record the rationale in the PR description, and redirect effort to Feature 2."

No code changes are required. The 'stale' designation is dropped. Effort is redirected to Feature 2 (broader test audit) which can now be correctly scoped given that telemetry-loop-integration.test.ts is confirmed current.

Changes

  • docs/artifacts/pln-352-drift-characterization.md: Written characterization artifact documenting drift class, cited production line numbers, and close-out rationale

Test plan

  • tsx --test apps/desktop/test/telemetry-loop-integration.test.ts — 6/6 pass
  • just desktop-lint — clean
  • Grep for spawnPtySession|spawnPty|pty\.spawn|node-pty — 0 matches in apps/desktop/src/ and apps/desktop/test/

🤖 Generated with Claude Code


Loop ID: 019dbbca-9718-7510-b62b-f181f46e3558
Artifact: https://app.closedloop.ai/implementation-plans/PLN-352

…st.ts

- Classifies telemetry-loop-integration.test.ts as Class C (not stale)
- Documents actual symphony-loop.ts spawn sites (lines 1836, 4349, 4374,
  4407, 4446, 4467, 4500) correcting PRD's inaccurate citations (1733,
  3697-3830)
- Documents telemetry emission sites (lines 2923, 3339, 3409, 4162,
  4253, 4517)
- Records close-out rationale: all 6 tests pass against current production
  code, no PTY migration ever existed in the codebase
- Confirms zero matches for spawnPtySession/spawnPty/pty.spawn/node-pty

Testing: tsx --test apps/desktop/test/telemetry-loop-integration.test.ts
  passes 6/6 tests; just desktop-lint clean

Risks: None identified

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@peterulsteen peterulsteen requested a review from a team April 23, 2026 19:35
@chatgpt-codex-connector
Copy link
Copy Markdown

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

@peterulsteen peterulsteen self-assigned this Apr 27, 2026
@peterulsteen peterulsteen reopened this Apr 27, 2026
@peterulsteen peterulsteen changed the base branch from main to symphony/prd-220 April 27, 2026 20:29
@peterulsteen peterulsteen merged commit 1ed1ae5 into symphony/prd-220 Apr 27, 2026
2 checks passed
@peterulsteen peterulsteen deleted the symphony/pln-352 branch April 27, 2026 20:29
peterulsteen added a commit that referenced this pull request Apr 30, 2026
…st.ts (#124)

- Classifies telemetry-loop-integration.test.ts as Class C (not stale)
- Documents actual symphony-loop.ts spawn sites (lines 1836, 4349, 4374,
  4407, 4446, 4467, 4500) correcting PRD's inaccurate citations (1733,
  3697-3830)
- Documents telemetry emission sites (lines 2923, 3339, 3409, 4162,
  4253, 4517)
- Records close-out rationale: all 6 tests pass against current production
  code, no PTY migration ever existed in the codebase
- Confirms zero matches for spawnPtySession/spawnPty/pty.spawn/node-pty

Testing: tsx --test apps/desktop/test/telemetry-loop-integration.test.ts
  passes 6/6 tests; just desktop-lint clean

Risks: None identified

Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
peterulsteen added a commit that referenced this pull request May 5, 2026
…st.ts (#124)

- Classifies telemetry-loop-integration.test.ts as Class C (not stale)
- Documents actual symphony-loop.ts spawn sites (lines 1836, 4349, 4374,
  4407, 4446, 4467, 4500) correcting PRD's inaccurate citations (1733,
  3697-3830)
- Documents telemetry emission sites (lines 2923, 3339, 3409, 4162,
  4253, 4517)
- Records close-out rationale: all 6 tests pass against current production
  code, no PTY migration ever existed in the codebase
- Confirms zero matches for spawnPtySession/spawnPty/pty.spawn/node-pty

Testing: tsx --test apps/desktop/test/telemetry-loop-integration.test.ts
  passes 6/6 tests; just desktop-lint clean

Risks: None identified

Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
peterulsteen added a commit that referenced this pull request May 5, 2026
…st.ts (#124)

- Classifies telemetry-loop-integration.test.ts as Class C (not stale)
- Documents actual symphony-loop.ts spawn sites (lines 1836, 4349, 4374,
  4407, 4446, 4467, 4500) correcting PRD's inaccurate citations (1733,
  3697-3830)
- Documents telemetry emission sites (lines 2923, 3339, 3409, 4162,
  4253, 4517)
- Records close-out rationale: all 6 tests pass against current production
  code, no PTY migration ever existed in the codebase
- Confirms zero matches for spawnPtySession/spawnPty/pty.spawn/node-pty

Testing: tsx --test apps/desktop/test/telemetry-loop-integration.test.ts
  passes 6/6 tests; just desktop-lint clean

Risks: None identified

Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
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