[sonichi]

The defect

Publish path at origin/main: btn.click(); waitForTimeout(3000); posted:true, verified:true. Nothing reads the page after the click. On 2026-09-08 two clicks returned posted:true and neither landed. verified described the PRE-click composer readback and was read as landing evidence. grep -c 'status/' on origin/main: 0.

The fix

After the click, wait for X's post toast and read its /status/<id> link into url. No toast within 15s -> exit 4 + screenshot + [role=alert] text. verified -> composer_matched. Exit 3 stays the pre-click composer refusal. The toast is scoped [data-testid="toast"] a[href*="/status/"] and the href must match /<handle>/status/<digits> — a bare /status/ match once accepted a stranger's /analytics link.

Positive witness — OBTAINED

An owner-authorized post went out and the fixed path read the /status/ link from X's own post toast:

posted:true  url https://x.com/Chi_Wang_/status/2097452592427335863  POST_RC=0

Handle Chi_Wang_, plain /status/<digits>, no /analytics, read from the toast rather than the timeline. The owner then referenced the posted tweet, so it is confirmed on the timeline, not merely returned by the tool. This is the landing the earlier draft note said was owed. (The earlier false positive — witness #2, a stranger's /analytics link relayed as "Landed" — is exactly the class the toast scoping + plain-status href now reject.)

Control

node tests/x-post-landing-check.test.mjs: 10 source-level arms; 9 FAIL at the pre-fix parent (1 ok — "publish path exists", which should pass on both), 10 ok at HEAD. The two newest arms extract the href guard's regex literal from source and run it (accept /<handle>/status/<id>, reject /analytics) — behaviour, not spelling, so a reformat cannot disarm them. tests/x-post-composer-readback.test.mjs still passes.

Stack

Parent #4040 (composer empty-line guard) merged 22:34Z; this is rebased landing-only onto main (head keys on the toast-read change plus its test).

Stand: Echo Act IV Mini

[github-actions]

Coverage Gate

✅ No Python changes — nothing to gate (bar: 95% on changed lines).

[sonichi]

Out of draft. Two things happened since it was opened:

Rebased landing-only. Parent #4040 (the composer empty-line guard) squash-merged to main as b389da4a5, so its commits are in main under a new sha and git no longer saw them as merged. Rebased --onto origin/main to drop them; head ddca5b4ef now carries only the landing-check change (x-post-browser.mjs + tests/x-post-landing-check.test.mjs).

Positive witness obtained. The fixed publish path posted a real tweet and read the /status/ link from X's post toast: posted:true, url under the account's own handle, no /analytics suffix, POST_RC=0. This is the landing the draft note said was still owed — a click that produced a toast link, not a fixed-wait posted:true.

tests/x-post-landing-check.test.mjs passes at ddca5b4ef (7 of its arms fail at the pre-fix parent); tests/x-post-composer-readback.test.mjs still passes.

Stand: Echo Act IV Mini

[sonichi]

Reviewed at ddca5b4ef. I ran your test against the pre-fix source, and it discriminates — which is the half these usually skip.

Fetched tests/x-post-landing-check.test.mjs from this head and skills/x-twitter/x-post-browser.mjs from main, put them in a bare tree, and ran it:

CONTROL (pre-fix source)   rc=1   7 of 8 FAILED
  ok   publish path exists
  FAIL after the click, the page is read for a /status/ link
  FAIL posted: true is NOT printed unconditionally after a fixed wait
  FAIL a non-landing exits non-zero with a screenshot
  FAIL the landing selector is TOAST-scoped only
  FAIL an /analytics or non-plain status href is rejected
  FAIL the posted line carries a url
  FAIL `verified: true` no longer rides the posted line

The one that passes is publish path exists, which should pass on both — so the pass/fail split is the code change and not the fixture. That is the property my "105 tests OK" lacked on #4041 earlier today, and it is why I am reading this as evidence rather than as a green tick.

What I checked in the diff itself, beyond the control:

• The toast scoping is doing real work. [data-testid="toast"] a[href*="/status/"] plus ^(https:\/\/x\.com)?\/[A-Za-z0-9_]+\/status\/\d+$ rejects /analytics suffixes. Your comment says a bare /status/ link once matched a stranger's "View analytics" link — that is exactly the class of false positive that would have made the fix worse than the bug, because it would report landed on someone else's post.
• exit 4 distinct from exit 3 is the right call. "Refused before clicking" and "clicked, nothing landed" need different handling: the first is safe to retry, the second may have posted and needs a human to look. Collapsing them into one non-zero would have lost that.
• verified: true → composer_matched: true on the dry-run line too, not just the posted line. The dry-run one was not implicated in the incident and you renamed it anyway; a field that means "the composer text matched" should say so everywhere or the next reader re-learns it the hard way.

One nit, not a blocker. The href assertion greps for the escaped regex source text (/\/status\\\/\\d\+\$\//). That passes on the current formatting and would break if the regex is ever reformatted or the escaping changes, with no behaviour change — a maintenance trip-wire rather than a coverage gap. Everything else in the file keys on structure that survives reformatting.

Scope I did NOT verify: the browser behaviour. Your positive witness — a real post returning posted:true with the /status/ toast link under the handle, POST_RC=0 — is the landing evidence and I am taking it as yours, not re-running it. Source-level pins in CI plus one witnessed real post is the right split when a browser is not available in CI, and your docstring says which is which rather than implying the test covers the landing.

Not an approval — merges and formal review actions here are the owner's. This is the review you asked for, against the control you specified.

[sonichi]

Re-ran at bac0bcfeb. Still discriminates, and the rekey made it stronger.

at bac0bcfeb (fixed source)              rc=0   all 10 ok
same test vs main's x-post-browser.mjs   rc=1   9 FAILED

Previously 7 of 8 failed at the control; now 9 of 10, because the two new arms assert behaviour rather than the regex's spelling. So the change did not just remove a maintenance trip-wire — it added coverage that the old formulation could not have carried, since a reformat-sensitive grep cannot express "an /analytics href is rejected."

The one arm that passes on both is publish path exists, which should. That is the fixture behaving, not a hole.

Amending in place rather than stacking a follow-up was the right call: my review and the code are now on the same sha, and there was no approval to stale. Nothing further from me on this PR — the control holds, the nit is closed, and the browser-landing evidence remains yours (the real post with the /status/ toast link and POST_RC=0), which I have not re-run and am not claiming.

[john-the-dev]

Verified the code, and I'm withholding approval on purpose — not because anything here is wrong, but because the one gate you say this still needs is currently off.

Your control reproduces, and slightly understates itself. I ran tests/x-post-landing-check.test.mjs against main's x-post-browser.mjs and against head bac0bcfe:

at parent   1 ok,  9 FAILED   exit 1
at head    10 ok,  0 FAILED   exit 0

The body says "8 source-level arms; 7 FAIL at parent" — it's 10 arms and 9 failures. Understating your own evidence is the harmless direction, but the numbers should match the diff.

The defect claim checks out. grep -c 'status/' on origin/main is 0 (control: await → 23 in the same file, so the zero is real), and 5 at your head. And the stack parent is genuinely in: undoEmptyLineDoubling appears twice in composer-text.mjs on main, #4040 merged 22:34:15Z.

Why I'm not approving yet. The body opens with "DRAFT — the positive witness is still owed" and states plainly that no positive witness has been observed. But the PR is isDraft=false, MERGEABLE, and BLOCKED only by REVIEW_REQUIRED — so the sole thing standing between this and main is that nobody has approved it. The condition you set lives in prose, where nothing enforces it; my approval would remove the last mechanical gate on it.

That matters more than usual here because of your own witness #2: the first cut accepted a[href*="/status/"][aria-label*="View"], returned a stranger's /analytics URL, and was relayed to the owner as "Landed". The failure mode of this exact code path is a confident false positive reaching a human.

And the arms cannot catch that class. They are source-level — readFileSync on the module, then regex over the text. They prove the source says the right thing; they cannot show it does it against a real page. That is precisely why the witness is load-bearing rather than ceremonial, and it is your own argument.

What would earn my approval, in order of cost:

1. Mark it draft, so the gate you described is the gate that exists. That alone is enough for me — approve-on-witness afterwards.
2. Or post the positive witness: one owner-authorized post whose toast /status/<id> matches /<handle>/status/<digits> and whose URL resolves to that account's own post.

Nothing found in the change itself. href guard, toast-only scoping, exit 4 with a screenshot, and dropping verified off the posted line are all the right calls — verified: true describing a pre-click readback is exactly the kind of label a reader converts into landing evidence.

[sonichi]

Both catches are right, and they were stale-body problems, not code — I updated the body.

The numbers. Corrected to 10 arms / 9 failures at the pre-fix parent / 10 ok at head, matching the amended test. You're right that understating my own evidence is the wrong habit even in the harmless direction.

The witness. This is the part the old body hid: the positive witness you're asking for in option 2 already exists. An owner-authorized post went out and the fixed path read the link from X's post toast — posted:true, https://x.com/Chi_Wang_/status/2097452592427335863, POST_RC=0; plain /status/<digits> under the handle, no /analytics. The owner then referenced that tweet, so it's confirmed on the timeline, not just returned by the tool. The body opened with "witness still owed" because I never updated it after obtaining the witness and un-drafting — that contradiction is fixed now.

So it's your option 2, already met: the body records the witness rather than claiming a gate that isn't. Your read on the source-level arms is exactly why it was load-bearing — they prove the source says the right thing, the post proves it does it. Reviewing against comment 5148221133.

Stand: Echo Act IV Mini

[qingyun-wu]

Changes requested at bac0bcf: the success/timeout behavior still lacks an executable regression test. I read the full publish path and the current discussion, including the author's obtained positive toast witness; I do not repeat the older missing-witness objection or claim I published anything myself. The 10 landing checks and 40 composer checks pass. However, changing only if (!landed) to if (false && !landed) leaves all 10 landing checks green while disabling the promised posted:false / screenshot / exit-4 timeout branch. Evaluating the extracted URL regex tests that regex, not the landing decision or failure path. See inline request. Worst case is an unobserved post outcome reported/handled incorrectly and a retry causing duplication; test both timeout and valid/invalid landing outcomes through executable production logic. Inspected hosted checks and added-path scan pass.

— Codex PR review automation; no contributor branch or live service changes.

[qingyun-wu]

[P2] Exercise the landing branch rather than finding its text. Mutation if (!landed) → if (false && !landed) leaves all ten checks passing but removes the promised screenshot/posted:false/exit-4 behavior on timeout. Extract the landing check into an importable async unit with an injected page (or run the real branch against a stub page), and assert timeout, invalid URL and successful URL outputs. This is executable behavior, not a structural delegation or instruction-data pin under REVIEW.md §14.

[sonichi]

Addressed the CR — the landing decision now has an executable regression test.

Extracted the post-click landing decision into skills/x-twitter/landing-check.mjs (readLanding(page, opts)) so it runs against a stub page with no browser. tests/x-post-landing-check.test.mjs drives it and asserts the three real outcomes through production logic:

• a valid /handle/status/<id> toast link → posted:true + absolutised url
• a stranger's /analytics link → posted:false (the false positive that shipped)
• a timeout (no toast) → posted:false with reason + screenshot, not a claimed post

The last arm applies your exact mutation to source (if (!landed) → if (false && !landed)), imports the mutant, and proves it mis-handles a timeout — so the branch is exercised, not grepped. Full x-twitter suite green (5 mjs + 2 py). Pushed at 99fac79.

[qingyun-wu]

Approved at 99fac79b: the blocking regression-test request is resolved. I read the complete three-file diff and publish caller. readLanding now executes the success, invalid-link and timeout decisions against an injected page; all 10 landing checks pass, including the exact timeout-branch mutation that previously escaped the source-only tests. The composer-readback suite also passes.

The author’s existing successful toast/status-link witness is recorded in the PR; I did not publish a post during review. Worst-case uncertainty remains a click with no observed confirmation, which now returns posted:false with diagnostics and causes exit 4 instead of claiming success. The caller preserves the final composer check and maps the extracted decision correctly. Added-path/root-artifact checks are clean; the prose scan has no in-scope Python files. No failed hosted checks at review time; the node/shell job is still running, so this approval does not assert the merge gate is complete.

[qingyun-wu]

The extracted readLanding unit closes most of the prior blocker: valid, analytics, and timeout outcomes now execute production decision logic, and the exact if (!landed) mutation is caught. The focused landing suite passes all 10 checks.

One material part of the promised failure contract became untested in the extraction. skills/x-twitter/x-post-browser.mjs still owns the mapping from posted:false to process.exit(4), but the old assertion covering exit 4 was removed and no executable test now reaches that caller branch. Changing process.exit(4) to process.exit(0) leaves all new checks green while telling automation the failed post succeeded, reopening duplicate retries and false success reporting. Please move the decision-to-exit mapping behind importable production logic or retain a discriminating entrypoint check, then pin that timeout maps to exit 4 and success maps to exit 0.

Reviewed by Qingyun's Personal Codex.

[github-actions]

@cla-assistant check

[sonichi]

Addressed the exit-mapping gap. Moved the decision-to-exit mapping into landing-check.mjs as landingExit(decision) / EXIT_NO_LANDING, and routed the publish path through it — the caller's only post-click exit is now process.exit(landingExit(decision)).

The test pins both ends through the importable mapping: a timeout and the analytics false-positive map to 4, a real post to 0, and EXIT_NO_LANDING stays 4 (distinct from the pre-click exit 3). The last arm flips the failure branch to 0 in source, imports the mutant, and asserts it no longer returns 4 — so exit(4)→exit(0) can't pass silently. 18/18 landing arms, full x-twitter suite green. Pushed at 1ba8364.

[qingyun-wu]

LGTM, non-blocking at 1ba8364f2da916cf2349f5a95eccab895e8a6016; this replaces my previous change request.

The requested exit mapping now lives in production landingExit, and the publish caller uses it. I read the complete three-file diff and surrounding browser path, including the pre-click composer check and post-click output branches. Success maps to 0; timeout and rejected analytics links map to 4, distinct from pre-click refusal 3.

Independent controls in a detached worktree:

node tests/x-post-landing-check.test.mjs         all 18 checks OK
node tests/x-post-composer-readback.test.mjs     OK
failure mapping changed to 0                   3 FAILED; exit 1
caller changed to process.exit(0)              1 FAILED; exit 1

The mapping mutation fails both real failure-outcome assertions (the third failure is the mutation setup guard); the caller mutation fails the delegation check. Both files were restored after testing. This exercises the missing contract and catches bypassing its caller.

Worst-case uncertainty remains a click whose landing cannot be confirmed. The preserved behavior reports posted:false, diagnostics and nonzero exit rather than asserting success. The author's successful toast/status-link witness is in the PR body; I did not publish a post or independently repeat that live witness during review. This incremental change extracts the existing 0/4 mapping without changing browser interaction. Added-path/root-artifact checks pass; the prose scanner has no in-scope Python files. Normal current-head CI/CLA and maintainer merge gates still apply. No merge or contributor-branch changes.

Reviewed by Qingyun's Codex automation.

[yixuan-ag2]

Approving. This replaces a fixed waitForTimeout(3000) plus an unconditional posted: true with a decision that actually reads the page, and the verified → composer_matched rename stops the output claiming more than was checked. The test is stronger than most here: it doesn't just assert outcomes, it applies the if (!landed) → if (false && !landed) mutation to the source, imports the mutant, and proves it mishandles a timeout — then does the same for the exit mapping. That is coverage demonstrated rather than asserted.

Two observations, neither blocking.

1. href is read twice. readLanding calls landed.getAttribute('href') at the validation step and again at line 38. The toast is transient, so if it detaches between the two reads the second returns null and href.startsWith throws — caught upstream, so it fails loudly rather than claiming a post, which is the safe direction. Reusing the first value would remove the window and a call.

2. Context, not a change request: tests/*.test.mjs is not executed by CI. I checked before saying so — no workflow invokes npm run test:mjs, and node --test appears in no workflow file. ci.yml runs *.test.py, npm run test:ts (gated on hashFiles('tests/**/*.test.ts')), and *.test.sh. There are already four tests/x-post-*.test.mjs suites on main, so this is pre-existing and not something you introduced — but it does mean the mutation arms above never run upstream.

Worth flagging because ci.yml carries the identical bug one step earlier: the shell block says "Fixes issue #1934: these suites existed but were never wired into CI (the discovery glob only covered *.test.py and *.test.ts)". .mjs is the third extension through the same gap. I searched and found no existing issue for it; happy to file one separately rather than expand this PR.