Skip to content

test(console): pin the long-name check against general backtracking and the unknown-outcome paths - #2945

Merged
freeqaz merged 2 commits into
mainfrom
ux-fixes-131-longname
Oct 19, 2026
Merged

freeqaz merged 2 commits into
mainfrom
ux-fixes-131-longname

Conversation

@freeqaz

@freeqaz freeqaz commented Oct 19, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

bughunt-129 audited #2943 and found three gaps in its tests:

  • 2031 (medium): the long-name overlap invariant compares the alternatives in UNBROKEN_ALTERNATIVES with each other, one character at a time. A second branch inside one alternative, an alternative that consumes more than one character, or an alternative added where they are joined (UNBROKEN_CHARACTER, RUN_CHARACTER) escapes it. Mutants that add [a-z] that way pass all 16 tests and bring back the finding-1999 freeze on a plain 210-character lowercase name (about 2 s, vs 0.1 ms on main).
  • 2032: fix(console): pin the long-name overlap invariant and drop the dead credential-saved line #2943 dropped the channel-Secrets panel's unknown-outcome line, relying on the deploy status to name it, but nothing asserted that it does. A mutant that skips it shows "Ready to deploy." and passes 117 tests.
  • 2033: nothing pinned that an unknown outcome locks the channel Secret pickers. Removing the lock passes all five channel-Secrets files.

Change

Tests only, plus one export:

  • apps/controller/src/console/agents/list.mjs: export the compiled rule, UNBROKEN_RUN, so the test can check its shape. No behaviour change.
  • tests/conformance/console-agent-name-runs.test.mjs:
    • three timing rows on ordinary names (plain lowercase, plain digits, letters before a hyphen), each a run of 20 that just fails to be marked, with the file's existing 50 ms bound;
    • each alternative, with its lookarounds removed, must be exactly one character class or one escape that consumes a character (no branch, no second character, no \b/\B or backreference);
    • the compiled rule must be the em-dash run, then exactly (?:(?:<alternatives joined>)\p{M}*) with {21,}, then the same run {20} plus one break-character class.
  • tests/browser/console-agent-draft-guards.test.mjs (the channel Secrets case of "Agent deployment waits for … writes and their recovery"): after the unknown outcome, the deploy status shows "Credential changes may have been saved. Reload this draft and inspect the saved state before deploying." (2032), and the Slack app and bot token pickers are disabled (2033). Six added lines in the existing case, which already sets up that state.

Verification

Mutant Killed by
alternative 0 becomes (?:[^…]|[a-z]) plain-name timing rows (~260-360 ms here) and the one-character check
|[a-z] appended at the join plain-name timing rows
|[a-z] inside RUN_CHARACTER plain-name timing rows
extra alternative -[⁠‍] one-character check
extra alternative [a-z][a-z] one-character check
extra alternative ́[a-z] one-character check
|[a-z][a-z] appended at the join (fast, so no timing row sees it) compiled-rule check
credentials.mjs deploy gate skips outcomeUnknown (2032) draft-guards channel Secrets case
canEnterChannelCredentials drops !state.outcomeUnknown (2033) draft-guards channel Secrets case
  • console-agent-name-runs.test.mjs: 20/20 at 2 CPUs (21 tests), before and after the review changes (internal review: two first-round timing rows could not see their overlap, and the escape branch was too permissive; both fixed, mutants rerun).
  • The touched draft-guards case: 20/20 at 2 CPUs; the full console-agent-draft-guards and console-agents-list-return files are green (44/44).
  • Prettier and eslint pass on the touched files.
  • Open PRs touching these files (feat: add per-agent sandbox network policies and denial review #2599, feat(console): stop and delete Agents from the Agents list #2524) and the parallel test-consolidation branch: no new conflict hunk (git merge-tree against main vs against this head: 33/33, 4/4, 0/0).
  • No UI change, so no media.

Risks

  • The compiled-rule check fails on any change to how the rule is assembled. That is deliberate (the composition is where the freeze came from), and its message points at the shape it expects.
  • The timing rows use the same 50 ms bound as the existing rows. Main takes well under 1 ms on them.
  • Findings: 2031, 2032, 2033 (bughunt-129). Not covered: bughunt's note that treating 500-504 as definitive rejections also survives these files.

…nd the unknown-outcome paths

The overlap invariant only compared alternatives with each other, so a
branch inside one alternative, a multi-character alternative, or an
alternative added at the join brought the finding-1999 freeze back on
plain names (finding 2031). Add timing rows for plain and mixed names,
require each alternative to consume one character, and check the
compiled rule joins exactly those alternatives (UNBROKEN_RUN is now
exported for that). Also assert that an unknown channel Secrets outcome
names itself in the deploy status (finding 2032) and locks the Secret
pickers (finding 2033).
@freeqaz
freeqaz requested a review from a team as a code owner October 19, 2026 15:04
@clawsweeper

clawsweeper Bot commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@clawsweeper clawsweeper Bot added clawsweeper:intake:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-environment Preliminary submission classification; full source review pending. clawsweeper:intake:needs-repro Preliminary submission classification; full source review pending. P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed clawsweeper:intake:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-environment Preliminary submission classification; full source review pending. clawsweeper:intake:needs-repro Preliminary submission classification; full source review pending. labels Oct 19, 2026
@clawsweeper

clawsweeper Bot commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge.

What this changes

This PR strengthens long-name regex guards and checks uncertainty messaging and disabled Secret pickers after a lost save response.

Example: A Slack credential save commits, but its response is lost.

  • Before: Existing tests check deployment blocking without asserting the deploy explanation or both picker locks.
  • After: The same browser case also requires the uncertainty explanation and disabled Slack app and bot token pickers.

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) Useful, focused coverage with no identified defect; confidence in execution rests on contributor-reported mutation and browser runs while current-head CI remains queued.
Proof confidence 🦐 gold shrimp (3/6) This collaborator-authored test improvement leaves runtime behavior unchanged; the inspected attachment screenshots show separate stop/delete work and do not establish coverage for this PR.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Product

Kind: Test only · Worth it: Yes · Fix scope: Complete
User problem: Future regressions could freeze the Agents list or leave uncertain credential saves with misleading guidance and editable pickers.
Reason: The area contributor documents concrete mutation survivors, and the additions extend existing coverage without changing product behavior.

Merge readiness

✅ Ready for maintainer review

Keep open: this collaborator-authored PR adds useful regression coverage, with no introduced correctness or security defect found.

Priority: P3
Reviewed head: 9c89d3a177a4ffedd8b6fd0ff625d524c2004e59

Before merge

None.

Findings

None.

Agent review details

How this fits together

The console checks Agent names before rendering wrapping styles and uses credential-save state to control deployment guidance and Secret editing.

flowchart TB
  A[Agent names] --> B[Long-name regex]
  B --> C[Wrapping marker]
  D[Credential save] --> E[Lost response]
  E --> F[Unknown outcome state]
  F --> G[Deployment guidance and picker locks]
Loading

Technical review

Best possible solution:

Land the focused regression guards while preserving existing long-name matching and uncertain-save behavior.

Do we have a high-confidence way to reproduce the issue?

This is a coverage improvement rather than a current runtime repair; the contributor reports mutation failures and repeated passing runs, which were assessed against source but not rerun during this read-only review.

Is this the best way to solve the issue?

The added assertions cover distinct gaps in existing tests, and the deliberate structural checks support the established non-overlap invariant.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against def181770d66.

Provenance checked

  • Long-name expression visibility and composition keeps the original intent (cf2cc2e: Match Chromium's measured wrapping behavior; later fixes established non-overlapping alternatives to avoid exponential backtracking.)
  • Existing overlap and uncertain-save regression coverage keeps the original intent (def1817: Protect the non-overlap invariant and retain one actionable uncertainty explanation after removing unreachable duplicate text.)

Testing

Proof path: in-process harness.

Security

None.

Evidence

What I checked:

Likely related people:

  • Free Wortley: Raw commit def1817 adds apps/controller/src/console/agents/list.mjs:304 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: def181770d66; files: apps/controller/src/console/agents/list.mjs)

Review metrics

Metric Value Why it matters
Production versus test changes Production +2/-1; tests +40/-0 The production change exposes the existing expression; test growth adds five conformance cases and extends an existing browser scenario.

Labels

Label changes:

No label changes.

Label justifications:

  • P3: Focused regression-test maintenance with unchanged runtime behavior.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR.

Rating scale

6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior.

Workflow

ClawSweeper edits this one comment on every review. Comment @clawsweeper re-review for a fresh review only. Repair and merge execution lanes are retired.

History

Review history (1 earlier review cycle)
  • reviewed 2026-10-19T15:17:35.070Z sha 167ca4d :: needs maintainer review before merge. :: none

Reviewed October 19, 2026, 11:43 AM ET / 15:43 UTC (Revision 2).

The letters-and-digits and hyphen rows could not see their overlap
(ten doubled characters, or one-character runs), so use twenty plain
digits and drop the hyphen row. The one-character check now accepts only
escapes that consume a character (not \b, \B or backreferences), and
property escapes with a value (\p{sc=Han}). Match the Secret picker
labels exactly.
@freeqaz

freeqaz commented Oct 19, 2026

Copy link
Copy Markdown
Collaborator Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

🦞👀
Exact review queued.

Re-review progress:

@clawsweeper clawsweeper Bot added clawsweeper:intake:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-environment Preliminary submission classification; full source review pending. clawsweeper:intake:needs-repro Preliminary submission classification; full source review pending. and removed clawsweeper:intake:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-environment Preliminary submission classification; full source review pending. clawsweeper:intake:needs-repro Preliminary submission classification; full source review pending. labels Oct 19, 2026
@freeqaz
freeqaz merged commit 067655e into main Oct 19, 2026
62 of 66 checks passed
@freeqaz
freeqaz deleted the ux-fixes-131-longname branch October 19, 2026 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant