fix(mt#1274): [SUPERSEDED by mt#1281] propagate sessionId to internal session_update

[minsky-ai]

Status: superseded — do not merge

This PR is redundant. While it sat in review, mt#1281 landed the same fix on main with the same approach (typed updateParams: SessionUpdateParameters declaration replacing the unsafe as cast). See commit ffe5a589e / merge 43d099bcc.

The diagnosis, fix shape, and reasoning are identical between the two PRs — parallel work, neither side knew about the other. mt#1281 also added a regression test in session-pr-workflow-bug.test.ts that I declined to write here (I judged the runtime test wouldn't bite reliably; mt#1281 found a way that does).

Recommended action: close this PR. mt#1274 already moved to CLOSED. No code action needed.

This is the second hit on the parallel-work pattern surfaced earlier in feedback_check_parallel_work_before_decomposing.md. Worth tightening the pre-dispatch search to also check open PR titles + bodies for the bug signature, not just task specs.

Had Claude look into it.

[minsky-reviewer]

Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: unknown

---

Findings

• [BLOCKING] Spec-implementation mismatch: skipUpdate flag is ignored in the updated Step 6 flow

  • Evidence:
    • Declared in schema: src/domain/schemas/session-schemas.ts: SessionPRParametersSchema includes skipUpdate: z.boolean().optional()
    • Implementation: src/domain/session/session-pr-operations.ts, STEP 6 block ("🔍 Checking for conflicts before PR creation...") always invokes updateSessionImpl unconditionally. There is no guard like if (params.skipUpdate) { /* skip update */ } before the call.
  • Failure mode: A caller providing --skip-update (or equivalent) expects the pre-PR update step to be skipped. Current behavior forces an update (including stash/pop/push side effects), violating the public schema contract and user expectation.
  • Requested change: Gate the update call with the flag, e.g., if (!params.skipUpdate) { await updateSessionImpl(...); } and make sure downstream logs reflect whether the update was skipped.

• [NON-BLOCKING] Schema validation defaults are not applied because the parsed result is discarded

  • Evidence:
    • src/domain/session/session-pr-operations.ts, STEP 0 calls SessionPRParametersSchema.parse(params) but does not reassign the returned value. Several fields in SessionPRParametersSchema (e.g., debug, draft, skipConflictCheck, autoResolveDeleteConflicts, noStatusUpdate) have default(false), but subsequent logic uses the original unparsed params.
  • Impact: Defaults from the schema are silently ignored. While this PR partially mitigates two booleans (see below), other defaults rely on ad-hoc fallbacks in this file, which is brittle and can drift over time.
  • Suggestion: Reassign the parsed value, e.g., const parsed = SessionPRParametersSchema.parse(params); and use parsed everywhere. This eliminates scattered manual defaulting.

• [NON-BLOCKING] Scope creep/undocumented change: behavior of two flags is normalized here instead of relying on schema defaults

  • Evidence:
    • src/domain/session/session-pr-operations.ts, STEP 6 argument object now uses skipConflictCheck: params.skipConflictCheck ?? false and autoResolveDeleteConflicts: params.autoResolveDeleteConflicts ?? false instead of direct pass-through.
  • Note: Because SessionPRParametersSchema already defaulted both to false, the more direct and type-safe approach is to consume the parsed object (see finding above). As-is, this creates a second place where defaults are encoded. Either update the PR description to mention this change or replace with the parsed params to avoid duplication.

• [NON-BLOCKING] Windows path handling bugs in session detection are still present

  • Evidence:
    • src/domain/session/session-pr-operations.ts:
      • STEP 1 checks currentDir.includes("/sessions/") to validate workspace
      • STEP 3 extracts sessionId by splitting on "/"
  • Failure mode: On Windows, backslashes are used, so detection fails even in valid session workspaces. This produces incorrect “must be run from within a session workspace” errors in CLI mode and fails to auto-derive sessionId.
  • Suggestion: Use path.sep, path.normalize, or a robust resolver (you already have resolve-session-directory.ts in this codebase) to make this platform-agnostic.

• [NON-BLOCKING] skipConflictCheck is never used by updateSessionImpl despite being plumbed through

  • Evidence:
    • src/domain/session/session-update-operations.ts: destructures skipConflictCheck: _skipConflictCheck but never references _skipConflictCheck afterward.
  • Failure mode: Users passing --skip-conflict-check see no effect; conflict checking still occurs (unless force is used). This is confusing UX and dead API surface.
  • Suggestion: Either honor this flag in the smart update path or remove it from SessionUpdateParametersSchema to avoid misleading configuration.

• [PRE-EXISTING] Title fallback logic is inconsistent with schema requirements

  • Evidence:
    • Schema: src/domain/schemas/session-schemas.ts requires title: z.string().min(1) for SessionPRParametersSchema (mandatory).
    • Implementation: src/domain/session/session-pr-operations.ts has logic to fill missing title/body from extractPrDescription if not provided.
  • Failure mode: Because parse(params) (STEP 0) enforces title, the later fallback block for missing title is effectively unreachable on a clean path. Either relax the schema to make title optional (if inference is intended) or remove the unreachable branch.

• [PRE-EXISTING] Hardcoded getCurrentSession: async () => undefined passed to updateSessionImpl

  • Evidence:
    • src/domain/session/session-pr-operations.ts, STEP 6 dependency object sets getCurrentSession: async () => undefined
    • updateSessionImpl resolves context with allowAutoDetection: !sessionIdParam
  • Note: Given an explicit sessionId is resolved earlier in sessionPrImpl, this is harmless, but it does disable any fallback detection in updateSessionImpl for this call path. If you ever choose to permit auto-detection in MCP mode, this will block it.

Spec verification

• Replace name: sessionId with sessionId in updateSessionImpl call: Met
  • src/domain/session/session-pr-operations.ts, STEP 6: the argument now includes sessionId instead of name
• Remove spurious json: false field: Met
  • src/domain/session/session-pr-operations.ts, STEP 6: json field is no longer present in the argument to updateSessionImpl
• Remove unsafe as SessionUpdateParameters cast to rely on structural typing: Met
  • src/domain/session/session-pr-operations.ts, STEP 6: the as import("../schemas").SessionUpdateParameters cast has been removed; excess property checks will now catch misnamed fields at compile time

Documentation impact

• CLI/MCP user docs:
  • Update the session PR command docs to reflect the behavior of --skip-update (currently declared in schema but ignored in implementation). Either document as unsupported or implement it as per the schema (recommended).
• Developer notes:
  • Consider adding a short note in architecture or code comments that parsed schema results must be used (not just validated) to ensure defaults are applied consistently.
  • If skipConflictCheck is intended, document its semantics and implement it in updateSessionImpl. Otherwise, remove from schema and docs.

Event: REQUEST_CHANGES

Rationale: The skipUpdate flag is part of the published parameter schema but is ignored in the Step 6 flow that this PR modifies; this is a user-visible behavior mismatch and should be addressed while the PR is already touching the relevant code. The remaining issues are non-blocking or pre-existing, but fixing the skipUpdate gating is small and directly in scope of the area being changed.