Report a broken import run instead of staying silent

[George-Payne]

AI-2405 - #749 (review)

What & why

The browser's Done screen waits for the machine's word rather than inferring an ending from the figures, so an import whose pass threw has to say so. It reports three zeros and the run_failed token: three counts cannot express "some unknown number is unaccounted", and the surviving pass's figures alone would state a clean import.

Where to look

Silence is the one option not available — it reads identically to a machine that died, which is a different ending, with different copy and half an hour of waiting between them. Partial counts are worse than zeros: the lost pass's skipped and failed are unknown and the route requires all three, so they would put a measured-looking zero where nobody measured.

run_failed is the one token that is not a refusal — sessions may have landed behind its zeros — so the token vocabulary and the lane's contract name it a failure, and copy saying the history was left alone is wrong for it. It also needs the server to know the token before this ships: reversed, the store refuses the report and the retry re-sends it every tick until the budget runs out.

Verification

Capacitor.Cli.Core.Tests.Unit → 2583 passed.

The throwing path pins the wire literal "run_failed" and three zero counts; mutating the constant to run_faled fails that assertion.

[linear-code]

AI-2405

[qodo-code-review]

ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

1. Commit subject lacks GitHub reference ✗ Dismissed 📘 Rule violation ⚙ Maintainability

Description

The commit subject Report a broken import run instead of staying silent does not end with the
required (#<digits>) GitHub issue reference.

Code

src/Capacitor.Cli.Core/FirstRun/BrowserFirstRunFlow.cs[491]

+            : Refusal(answer.DecidedAt, FirstRunImportOutcomeReasons.RunFailed);

Evidence

PR Compliance ID 2897961 requires every commit subject to end with exactly one GitHub issue
reference in the form (#<digits>); the supplied commit subject has no such reference.

Rule 2897961: Enforce single-clause imperative commit subject with GitHub issue reference and 80-char limit

---

2. PR reference line lacks GitHub ✗ Dismissed 📘 Rule violation § Compliance

Description

The PR description's reference line contains only AI-2405; it must include a GitHub closing
reference and the Linear key together on that line, such as Closes #123 AI-2405.

Code

src/Capacitor.Cli.Core/FirstRun/BrowserFirstRunFlow.cs[491]

+            : Refusal(answer.DecidedAt, FirstRunImportOutcomeReasons.RunFailed);

Evidence

PR Compliance ID 2897991 requires the reference line to contain exactly one GitHub closing keyword
and issue reference alongside at least one Linear issue key; the supplied description starts with
only AI-2405.

Rule 2897991: PR description must contain both GitHub and Linear issue references; PR title must not contain issue IDs

---

Context sources

✅ Compliance rules (platform): 55 rules

Review mode: ⚖️ Balanced

Tip of the day

💡 Did you know, you can add REVIEW.md to your repo root and Qodo follows it on every PR

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[alexeyzimarev]

Standards

• [P2] Align the contracts with run_failed's non-refusal semantics. The merged server contract explicitly makes this the one reason that is not a refusal: surviving passes may have landed sessions even though the wire counts must be zero because the totals are unaccounted. BrowserFirstRunFlow.cs:491 now constructs it through Refusal, whose comment at lines 509-513 says it is always a refusal; FirstRunImportOutcome.cs:3-9 and FirstRunFlowModels.cs:117-118 say every reason belongs only to a run that moved nothing; and IFirstRunImportLane.cs:42-45 still says a null result makes the caller send nothing. These contracts are now false. Please generalize the helper/name and update the comments to distinguish zero reported counts from actual sessions that may have landed.

• [P3] Rewrite the PR description as current-state rationale. Its “reported nothing… That held… it becomes…” and “the suite carried a test asserting the old silence… now pins…” passages are change narration and a test-diff inventory, both forbidden by .github/PULL_REQUEST_TEMPLATE.md. State the current constraint and verification result directly.

• [P3] Reduce the duplicated test doc comment. BrowserFirstRunFlowTests.cs:1573-1577 repeats rationale already carried by the production comments and by the test's name/assertions. CLAUDE.md says comments are scarce, one or two lines is the norm, and longer blocks must contain something unavailable elsewhere.

The existing thread already covers the missing GitHub half of the reference line, so I have not duplicated that finding here.

Spec

• [P3] Pin the server token independently. BrowserFirstRunFlowTests.cs:1591 compares the emitted reason with the same FirstRunImportOutcomeReasons.RunFailed constant production uses. A mistyped wire literal would keep this test green while the server rejects every report and the client retries until timeout—the rollout failure the spec explicitly warns about. The verification claim says this test pins the run_failed token and three zero counts; currently only the counts are independent. Assert the literal "run_failed" here or add equivalent wire-serialization coverage.

Validation: dotnet build Capacitor.slnx --no-incremental succeeded with 0 errors and 0 warnings; all 102 BrowserFirstRunFlowTests passed locally; GitHub reports all 6 CI checks passing.

Summary: Standards — 3 new findings, worst P2 stale contracts. Spec — 1 P3 finding, missing independent wire-token coverage.

[George-Payne]

All four taken, in 4b098c0 plus a description rewrite.

P2, the stale contracts. Correct, and the merged server contract already draws the line the CLI half was missing — FirstRunImportOutcomeReasons.RunFailed there says "A failure, where the two above are refusals". Four sites now match it:

• Refusal(...) → ReasonOnly(...), and its doc says the zeroes are the wire's requirement rather than a claim that nothing landed.
• The reasons class doc names what the token explains as the missing figures, and separates RunFailed from the two that did leave the history alone.
• RunFailed's own doc carries the failure-vs-refusal split.
• ReportFirstRunImportOutcomeRequest.Reason says "only alongside three zeroes" rather than "only on a run that moved nothing".
• IFirstRunImportLane.ImportAsync's returns doc no longer promises silence on null — it names the token the caller sends.

Spec, the wire token. Good catch, and the failure mode you describe is the rollout one. The assertion is now the literal "run_failed". Verified it bites: mutating the constant to run_faled fails it, where the constant-to-constant compare stayed green.

P3 description rewritten as current-state constraint, and P3 test comment cut to the two lines that say what the test pins.

Capacitor.Cli.Core.Tests.Unit → 102 BrowserFirstRunFlowTests pass on the new head.

[alexeyzimarev]

Standards

• [P2] Finish separating reported counts from actual sessions landed. src/Capacitor.Cli.Core/FirstRun/FirstRunImportOutcome.cs:8-10 still says the server “rejects any token at all on an outcome that moved something.” That contradicts the new RunFailed contract immediately below: sessions may have landed even though the report deliberately carries zeroes. The server can only reject a reason alongside non-zero reported counts; it cannot establish that the underlying run moved nothing. Please use that precise wording here, matching FirstRunFlowModels.cs:117-118.

The other standards findings from the first review are addressed: ReasonOnly replaces the misleading Refusal name, the lane/model contracts are corrected, the test comment is focused, and the PR description now states current constraints and evidence.

Spec

No findings. The regression test now independently pins the literal "run_failed" and all three zero counts, and the implementation still retains/retries the reason-only outcome until the server records it.

Validation: dotnet build Capacitor.slnx --no-incremental succeeded for all 12 projects with 0 errors and 0 warnings; all 102 BrowserFirstRunFlowTests passed locally; all 6 GitHub CI checks pass. One preceding focused-test launch exited once with native code 139 before producing test output; the immediate identical rerun passed.

Summary: Standards — 1 remaining finding (P2 stale contract wording). Spec — clean.

[George-Payne]

Taken, in 2c93c16 — and you were right about the wording being the precise point rather than a nicety. The store's guard is outcome.Total > 0, a sum of the three reported figures, so non-zero counts is exactly and only what it can judge; whether the underlying run moved anything is not visible to it.

That <para> now reads "It rejects a token it does not know, and any token that arrives on non-zero counts", matching FirstRunFlowModels.cs.

Sweeping the phrase rather than the line found one more instance, in my own previous commit: ReasonOnly's doc still said "if a reason arrives on counts that moved something". Same fix applied there.

Two other hits in the same sweep I left alone deliberately, since neither conflates the two — both describe a run that genuinely did move nothing: the import-outcome route's "an outcome that moved nothing is still worth sending" (why a zero-count report is sent at all, as the finished signal), and ImportCommand.ReportNothing's "a run that deliberately moved nothing".

Comment-only, so the evidence is the build: Capacitor.Cli.Core succeeds with 0 warnings, which is what checks the <see cref>s.

[alexeyzimarev]

Standards

• [P3] State why the GitHub half of the reference line is absent. The body starts with only AI-2405. CLAUDE.md requires both the Linear issue and a GitHub closing reference; .github/PULL_REQUEST_TEMPLATE.md permits dropping a nonexistent half only when the omission is stated. A repository issue search finds no AI-2405 match, so please make that explicit (for example, AI-2405 — no GitHub issue) or add the closing reference if one exists.

No code-hunk violations or baseline smells found. Commit 2c93c16e correctly resolves the remaining stale contract wording: both affected comments now describe the server guard in terms of non-zero reported counts.

Spec

No findings. Both null-return and thrown-import paths now owe run_failed with three zero counts; rejected delivery remains retained and retried without rerunning the import; the test independently pins the "run_failed" wire literal; and the known-token set includes it. The merged server-side AI-2405 change defines the matching token and semantics.

Validation: reviewed head 2c93c16e; all 6 GitHub CI checks pass. Per request, no local tests or builds were run.

Summary: Standards — 1 P3 PR-metadata finding; Spec — clean.