Post-Merge Review Findings for PR #830
PR #830 (Watch Next-Gen) merged with strong architecture but several issues that need follow-up fixes.
P0 — Must Fix
-
CLI flags not wired to config — --notify-level, --overnight-start/end, --sentinel-file\ are parsed from args but never passed to \loadWatchConfig(). These flags have no effect at runtime. Fix: add them to the \cliOverrides\ object.
-
\isProcessAlive()\ EPERM bug — On Unix, \process.kill(pid, 0)\ throws EPERM when the process exists but caller lacks permission. Current code treats any error as 'dead'. Fix: check \�rror.code === 'EPERM'\ → return true (alive).
P1 — Should Fix
-
Path traversal in WorktreeBackend —
elativePath\ parameter in
ead()/\write()\ is not validated for ..\ segments. Fix: validate or scope the FSStorageProvider.
-
Auth status stderr parsing — \gh auth status\ writes to stderr even on success. \�xecFileSync\ only returns stdout → auth drift detection broken. Fix: use \spawnSync\ or \gh api user -q .login.
-
*\invokeFleet()\ ignores --agent-cmd* — Fleet dispatch hardcodes 'copilot'\ instead of using \config.agentCmd. Breaks environments using wrapper commands.
-
--state-backend\ not validated — Invalid values are saved to config and silently fall back at runtime. Fix: validate upfront against allowed values.
-
SDK console.log — \github.ts:249\ logs directly to stdout. SDK should return status; CLI decides how to log.
P2 — Nice to Have
- BOM characters in cleanup.ts and cleanup-ceremony.md
- Changeset docs say PID in .squad/\ but implementation uses \os.tmpdir()\
- No integration tests for end-to-end watch flow
- Fleet dispatch has no tests
Copilot Comment Validity
11/12 Copilot comments were valid — very high signal-to-noise ratio on this PR.
Priority: P0 items should be fixed ASAP — users setting CLI flags will get no behavior change.
Post-Merge Review Findings for PR #830
PR #830 (Watch Next-Gen) merged with strong architecture but several issues that need follow-up fixes.
P0 — Must Fix
CLI flags not wired to config — --notify-level, --overnight-start/end, --sentinel-file\ are parsed from args but never passed to \loadWatchConfig(). These flags have no effect at runtime. Fix: add them to the \cliOverrides\ object.
\isProcessAlive()\ EPERM bug — On Unix, \process.kill(pid, 0)\ throws EPERM when the process exists but caller lacks permission. Current code treats any error as 'dead'. Fix: check \�rror.code === 'EPERM'\ → return true (alive).
P1 — Should Fix
Path traversal in WorktreeBackend —
elativePath\ parameter in
ead()/\write()\ is not validated for ..\ segments. Fix: validate or scope the FSStorageProvider.
Auth status stderr parsing — \gh auth status\ writes to stderr even on success. \�xecFileSync\ only returns stdout → auth drift detection broken. Fix: use \spawnSync\ or \gh api user -q .login.
*\invokeFleet()\ ignores --agent-cmd* — Fleet dispatch hardcodes 'copilot'\ instead of using \config.agentCmd. Breaks environments using wrapper commands.
--state-backend\ not validated — Invalid values are saved to config and silently fall back at runtime. Fix: validate upfront against allowed values.
SDK console.log — \github.ts:249\ logs directly to stdout. SDK should return status; CLI decides how to log.
P2 — Nice to Have
Copilot Comment Validity
11/12 Copilot comments were valid — very high signal-to-noise ratio on this PR.
Priority: P0 items should be fixed ASAP — users setting CLI flags will get no behavior change.