Skip to content

fix(extension): make #249 AC8 true — repair the bug-session test suite and wire the session-level model pin - #278

Merged
jeonghun-jj-lee merged 1 commit into
mainfrom
dev/gennadiryan/bug-fix-fix
Aug 8, 2026
Merged

fix(extension): make #249 AC8 true — repair the bug-session test suite and wire the session-level model pin#278
jeonghun-jj-lee merged 1 commit into
mainfrom
dev/gennadiryan/bug-fix-fix

Conversation

@gennadiryan

Copy link
Copy Markdown
Member

Important

Problem

main is red. Two extension tests fail at HEAD, both in the bug-session feature that HEAD itself landed, so #249's acceptance criterion 8 ("extension unit tests cover orchestration and lifecycle") is not met by the merged implementation. One failure masks a third: the suite asserts a model handoff on the arming turn that was never implemented.

Approach

Repair the two failures, then implement the session-level half of the model handoff the tests already specify — a configuration-pinned model on the arming turn, omitted when unset. The composer-level half is deliberately deferred to #277. Key reason: the tests are the spec here and they are unambiguous; aligning them downward to a partial implementation would narrow coverage in the very PR meant to make AC8 true.

Approaches Considered

  • Repair both failures, implement the session-level handoff, defer the composer half (chosen) — makes AC8 true without smuggling cross-boundary scope into a test-fix PR.
  • Repair the failures only, deleting the model assertion — smallest diff, but removes the last executable record of an intended design while claiming to restore coverage.
  • Implement both halves here — coherent, but the composer half crosses the iframe trust boundary and deserves its own decision surface; now BUG: Carry the composer's live model selection onto the bug session #277.

Scope

  • In: the stale create-body assertion; the missing test import; a configuration-pinned model on the arming turn; two new tests for the omitted-model branches.
  • Out: the composer→bridge model handoff — deferred to BUG: Carry the composer's live model selection onto the bug session #277 with the reasoning recorded at the dependency declaration. Unpinned-install behaviour — unchanged, the server still resolves its own default.

Assumptions / Open Qs

  • Assumes the arming turn is the right carrier for the model rather than session creation; the route's shape supports both, and the model is a property of the turn.

What was failing

F1 — the test never ran. The bridge test file calls the model extractor six times but never imported it, so the file raised a reference error before evaluating a single assertion. It has never passed.

F2 — the assertion was stale. Bug-session creation sends a question permission deny-list (per the lifecycle ADR: the dock owns dialogue through its textarea, not the question tool's structured Q&A). Production has sent it since the feature landed; the expectation was never updated. Production was correct.

F3 — masked by F2. With F2 repaired, a third failure surfaced: the suite expects a model on the arming turn. Production sent none. This was invisible because the earlier assertion threw first.

What changed

  • The create-body expectation now includes the permission deny-list.
  • The bridge test imports the extractor it exercises, so its six assertions run for the first time.
  • The bug-report orchestrator takes an optional model dependency, resolved from the existing configuration pin, and includes it on the arming turn when set.
  • Two new tests lock the negative branches: unset and whitespace-only both omit the field entirely.

Verification

The session command route was checked against the built binary's own OpenAPI document rather than assumed: it takes a required command and arguments, and optional model and variant, both strings. The test's expectation was well founded; only the implementation was missing.

  • Extension suite: 898 passing / 2 failing → 902 passing / 0 failing
  • Workspace total: 1994 passing, 0 failing
  • Typecheck clean across all three packages

Key Decisions

  • Precedence. Only an explicit configuration pin sets a model; otherwise the field is omitted and the server resolves its own. This matches the pin rule already used at boot and restart — the extension never guesses a model.
  • Omit, never empty. An empty string would pin the session to a nonexistent model. Both new tests assert omission rather than an empty value.
  • The model rides the arming turn. Session creation carries identity and the context envelope; the model is a property of the turn that runs.
  • The deferred half is documented where it will be looked for — at the dependency declaration, not only in the tracking issue.

Constraints & Invariants

  • Sanitization is unchanged: run context stays pointer-only, no absolute paths, no log payloads.
  • The single-open bug-session invariant and the machine-managed lifecycle are untouched.
  • The bridge command allowlist is unchanged in size.

Source

Notes

Until #277 lands, a bug session runs the configured or server default, so model attribution in a filed report reflects configuration rather than what the user was running. Worth knowing when triaging anything filed through the feature.

The sweep that surfaced this also produced diagnoses for #261 (paste duplicates input) and #266 (zoom keybindings), both of which are fork-side and belong in a separate change.

@gennadiryan
gennadiryan marked this pull request as ready for review August 7, 2026 22:14
@jeonghun-jj-lee
jeonghun-jj-lee merged commit b2ae458 into main Aug 8, 2026
6 checks passed
@jeonghun-jj-lee
jeonghun-jj-lee deleted the dev/gennadiryan/bug-fix-fix branch August 8, 2026 06:12
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.

2 participants