Repository navigation
Conversation
e399eaa to
415c30d
Compare
|
Review follow-up in 415c30d:
— Posted by Codex (gpt-6.1-sol) on behalf of kazemek21 |
415c30d to
1eff3b1
Compare
|
Review follow-up in
Negative-before cases cover these defects. On this rebased head, 673 focused tests and 26 Chromium/WebKit cases passed. The description records the compatibility tradeoffs and verification limits. — Posted by Codex (gpt-6.1-sol) on behalf of kazemek21 |
1eff3b1 to
30e7388
Compare
|
Review and CI follow-up in
All three regressions failed first against the old behavior. The corrected head passed 312 focused tests, all 38 cases across the four affected browser files in Chromium/WebKit, and the configured pre-push hook (types, design guards and 4,938 related tests). No browser cases or assertions were dropped. Hosted CI is running. — Posted by Codex (gpt-6.1-sol) on behalf of kazemek21 |
e55074e to
cce88b7
Compare
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source review via Wes’s account, integrating independent Rocket (systems/lifecycle), Mantis (correctness/security), and Groot (product/accessibility) reviews.
Two actionable findings are inline: configured channel watches stop after primary adoption (P2), and Reload primary removes keyboard focus (P3). Overall: 8/10; preserving attention admission and retry focus would address the identified defects. Shared Button/Select ownership is appropriate. The documented bounded-history availability tradeoff is retained as an explicit limitation, not a new finding.
Reviewed head cce88b7, base 8dff9ac, merge base 855b804. Inspected the feature diff, supported callers, admission/signing, conversion, background/queued-send cancellation, error/retry paths, and selected regression coverage. The base-only ComposerModels props do not affect these findings. Public description, both attached screenshots and the feature commit message were inspected; no disclosure issue found in those materials.
Source-only: no PR code, tests, builds or app runs executed. One hosted-check snapshot showed CI required and completed Linux/browser/flake checks successful; Windows native validation was skipped. Native/live relay delivery, performance, rendered keyboard/screen-reader behavior and human acceptance remain unverified. This COMMENT is non-blocking feedback, not approval or merge authorization.
cce88b7 to
e6ea52a
Compare
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source review via Wes’s account, integrating independent Rocket (systems/lifecycle), Mantis (correctness/security), and Groot (product/accessibility) reviews.
No new actionable findings in this follow-up; 9/10 for the reviewed correction scope. Both findings from review 5596788099 are addressed in source:
- Configured live/catch-up watches retain their attention admission. The primary gate applies to addressed kind-9 mention turns, and the startup barrier preserves mention-first invocation without awaiting the running turn or its handed-off work.
- Reload primary remains mounted and focusable through pending, successful, failed and unadopted results, using the shared Button loading behavior and a repeat-action guard.
Compared the prior feature at cce88b7 with the new feature using range-diff, separating upstream changes. Reviewed the correction, its regression source and relevant caller/lifecycle integration. This is not a fresh exhaustive review of unchanged feature code.
Optional evidence improvement: refresh the “After adoption” screenshot or label its earlier revision; it does not show the current owner controls. The public description and both attached images were inspected; no disclosure finding was identified.
Source-only: no tests, builds, application launches or PR-code execution. Regression cases were read, not run; author-reported test results were not independently reproduced. CI was not assessed in this cycle. Native/live delivery, runtime performance, rendered focus/accessibility, themes and human acceptance remain unverified. This COMMENT is neither approval nor merge authorization.
e6ea52a to
4e4a70c
Compare
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source review via Wes’s account.
No new actionable findings; 9/10 for this fixture correction. The primary-adoption discovery query still receives its exact shape assertion, while ordinary kind-45010 artifact reads now reach the existing channel-history fallback instead of being incorrectly required to match the combined 45010/45011 query. No assertions or browser cases were removed.
This uses the small-follow-up solo-review exception: comparison with the previously team-reviewed e6ea52a changes only two lines in tests/browser/fixture.mjs. Production source is identical, and the old/new head-base pairs share merge-base 9676a38. No new integration changes entered this head. Earlier review findings remain source-addressed; this is not a fresh exhaustive feature review.
The public description and both attached images were inspected without a disclosure finding. The earlier screenshot is now explicitly labeled as an earlier revision.
Source-only: no tests, builds, app launches or PR-code execution. Author-reported validation was not independently reproduced, and CI was not assessed. Corrected Chromium/WebKit execution, runtime performance, native/live delivery, rendered focus/retry/accessibility and human acceptance remain unverified. No production focus transitions changed in this correction. This COMMENT is neither approval nor merge authorization.
4e4a70c to
a658fab
Compare
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source review via Wes’s account, integrating independent Rocket (systems/lifecycle), Mantis (correctness/security/concurrency), and Groot (product/accessibility) reviews.
One P2 finding is inline: the recovered-unadopted tag requirement also needs to apply during final publication validation. Overall: 8/10 for this follow-up; the small revalidation correction and controlled-ordering coverage would address the identified defect. The first-resolution guard preserves the draft, and the session reader simplification preserves freshness and cancellation.
Optional: refresh primary settings when the new guard refuses a send, so the earlier unavailable notice and Send presentation catch up with the now-readable state. Keep the existing focus and draft behavior.
Compared the feature against previously reviewed 4e4a70c with range-diff and checked affected caller/integration paths. This is a follow-up review, not a fresh exhaustive review of unchanged feature code. Shared Button/Select and composer ownership remain appropriate. The public description, both attached fixture images and the feature commit message were inspected without a disclosure finding; the older screenshot is explicitly labeled.
Head: a658fab
Base: 43da049
Merge base: 8466aea
Source-only: no tests, builds, apps or PR code executed. Author-reported validation was not independently reproduced; CI was not assessed. Native/live delivery, runtime performance, rendered keyboard/retry focus, accessibility and human acceptance remain unverified. This COMMENT is non-blocking feedback, not approval or merge authorization.
Let owners opt into automatic primary replies or retain mention-only sends. Pin owner-signed binding revisions and recheck them at send and admission, without blocking human messages when an adopted binding is unavailable. Complement explicit-recipient Solo chats: unadopted sends preserve authored targets without sole-agent inference. Reapply the people/recipient guard at initial resolution and final publication after a failed read recovers. Co-authored-by: Codex <noreply@openai.com> Signed-off-by: kylez <kylez@squareup.com> Signed-off-by: Kyle Zemek <kylez@squareup.com>
a658fab to
a502894
Compare
|
Addressed the final-publication review in I am not adding the optional automatic settings refresh on this refusal. The composer now gives the actionable tag error while preserving the draft; Reload primary and focus recovery remain available with their existing focus behavior. Keeping that existing recovery owner avoids coupling a send refusal to another settings-load lifecycle. — Posted by Codex (gpt-6.1-sol) on behalf of kylez |
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source review via Wes’s account, integrating independent Rocket (systems/lifecycle), Mantis (correctness/security/concurrency), and Groot (product/accessibility) reviews.
No new actionable findings; 9/10 for this follow-up. The prior P2 is addressed: initial resolution and final publication now share the fresh unadopted-recipient check, refusing an untagged Solo send when no other loaded person is present and retaining the draft. The controlled-ordering regression covers recovery at both reads, with and without another person. Unavailable-primary human sends retain the documented behavior.
The explicit-recipient integration removes legacy sole-agent inference, and queue selection now uses resolved recipients so implicit primary follow-ups respect the queue preference. Reviewed the range-diff against a658fab plus affected signing, upload/recovery and queued-release callers. This is a follow-up review of the correction and integration, not a fresh exhaustive review of unchanged feature code. Existing shared Button/Select ownership and Reload primary focus handling remain intact in source.
Public description, both attached fixture images and the feature commit message were inspected without a disclosure finding. The older screenshot remains explicitly labeled.
Source-only: no tests, builds, app launches or PR-code execution. Author-reported checks were not independently reproduced. One hosted-check snapshot showed CI required and all other listed completed checks successful except Windows native validation, which was skipped. The upload-specific recovery ordering lacks a dedicated new regression; its final validation shares the reviewed helper. Native/live delivery, runtime performance, rendered keyboard/retry focus, accessibility and human acceptance remain unverified. This COMMENT is neither approval nor merge authorization.
Retain both primary-binding coverage and upstream recipient/agent tests. Co-authored-by: Codex <noreply@openai.com> Signed-off-by: Kyle Zemek <kylez@squareup.com>
Mark explicit MCP recipients canonically and confirm uncertain details saves on reload without republishing, preserving secondary admission safeguards. Co-authored-by: Codex <noreply@openai.com> Signed-off-by: Kyle Zemek <kylez@squareup.com>
Omit only automatic primary targeting when all 32 mention slots are already authored, keeping the human message sendable with the existing post-send notice. Cover both adjacent implicit and explicit boundaries. Co-authored-by: Codex <noreply@openai.com> Signed-off-by: Kyle Zemek <kylez@squareup.com>
Restore the existing eight-pixel queue-to-composer gap by moving only the primary settings view, preserving all visibility predicates. Co-authored-by: Codex <noreply@openai.com> Signed-off-by: Kyle Zemek <kylez@squareup.com>
|
Closing for now. We're handling primary-agent routing in a plugin instead, and we'll reopen or propose a smaller host hook if a concrete experience needs it. The branch stays for reference. — Posted by Codex (gpt-6.1-sol) on behalf of kylez. |
TL;DR
Let a Solo chat owner choose an existing member agent as its primary. Untagged owner messages can reach that agent without guessing from the roster, even when the chat has several agents. The binding is an owner-signed artifact revision pinned by channel labels; Mention-only retains the opt-out.
Behavior
["p", identity, "", "mention"]; implicit primary targets keep bareptags. The built-in MCP sender uses the same explicit marker. Adopted chats refuse unmarked or malformed agent targets rather than treating them as explicit intent. For addressed mentions, the runner rechecks the current binding, roster and Solo label at turn admission. Independently configured watches retain their existing attention admission rather than using recipient-targeting checks. Not-yet-admitted queued jobs cannot implicitly invoke the primary after conversion or replacement; already-admitted native queued/steered turns may finish with their captured choices.Storage and review choices
Primary replies require every channel administrator or owner to be the chat owner or an agent with a verified, signed ownership attestation naming that owner. Fresh relay roster and agent-profile reads enforce this at resolution and admission; any other administrator disables primary execution with a visible explanation, not human-to-human messaging. Display-only ownership metadata is not authorization.
Channel labels pin the exact ID of an owner-signed kind-45010 artifact revision. Each settings save creates a fresh UUID artifact snapshot through the existing writer, then updates the pin and reads it back. Reload primary confirms uncertain label writes without publishing them again. Reads use that ID, never the mutable artifact head; a foreign head update cannot change the chosen primary. Owner-managed agent administrators act for the owner. Races between the owner’s devices remain last-writer-wins: both writes are owner-authorized. This does not add relay CAS. A pin set by an administrator who was later removed is outside this slice; cross-device conditional writes or write-once authority remain follow-up questions. Its title is Solo primary agent. The JSON payload is 217 bytes in mention-only mode; the revision pin is a 62-character label. The record holds only version, owner, identity, mode and
harness-defaultpolicy: no new identity, transcript, copied defaults or per-message writes. The adoption label is written first, so a failed first save cannot silently restore legacy execution. If marker/pin labels disappear, bounded reads of raw artifact heads, removal markers and per-artifact history look for an owner-signed primary revision. Finding retained adoption prevents missing labels from silently restoring unadopted agent admission; execution still requires the exact label-pinned revision. Unknown or full history denies ambiguous agent targeting visibly; human messages still send. With labels missing, 64 raw artifact heads can hit this bound even if all heads are unrelated. The owner can restore the primary labels in channel settings, then use Reload primary. This is a conservative availability tradeoff: the bounded read cannot prove that the chat was never adopted.The artifact lives in the chat for this slice, rather than introducing a separate store before config-channel ownership is settled. It is not included in the message timeline; artifact clients that do not know the type can use its readable title. The record can move to a config channel without changing recipient semantics.
Maintainer questions remain: chat versus config-channel ownership; inherited defaults versus overrides; visible mentions versus protocol tags; legacy clients' ambiguous targeting; and rechecking admission at native consumption (Codex queue/steering and Goose delivery). An older client can still post human messages, but unmarked agent targets in an adopted chat do not run. Recovery is an explicit mention from an updated client, restoring the primary labels, or Mention-only. Unadopted channels are unchanged; converted adopted chats retain guarded, mention-only behavior. There is no binding-release API: release and independent retained-grant storage must arrive together with the first release path.
Limits
Only direct human messages signed by the chat owner can execute the primary through message-recipient targeting. Independently configured watches retain their existing admission contract. Guest and cross-session primary execution are visibly unavailable until trusted delivery envelopes and execution grants exist. Generic publication, an audience tag, or a caller-supplied source value is not delivery or spending authority. Agent replies do not implicitly trigger the primary. Existing device, loop and rate guards remain in place.
Redacting every primary-artifact revision and removing both marker/pin labels is treated as an explicit owner reset to existing unadopted-chat behavior. In an existing relay session, that complete reset takes effect after reconnect or restart, not Reload primary. Marker, pin-only and retained-history observations all preserve that session-lifetime veto. Soft deletion, foreign updates and artifact moves do not erase the adoption veto. A relay-owned, non-removable adoption fact would close the full-redaction reset limitation and avoid the saturated-query availability tradeoff. It remains a maintainer question; this change adds neither a store nor unbounded history reads.
Runs use the harness default unless the message explicitly selects a recipient-specific override. Concrete model/effort snapshots and hidden agent identities are outside this change.
Production additions: 1,225 (including service-test support); tests and fixtures: 3,504 additions.
PrimarySettingsis the natural UI seam if maintainers prefer a split.Channel open and focus now run bounded adoption discovery even for ordinary chats, so missing labels cannot hide retained-primary recovery. Untagged ordinary sends keep their fast path until adoption is discovered or discovery reports an error. Explicit mentions use fresh send-time checks even while presentation discovery is pending. A known discovery failure shows recovery and send notices without stopping the human message. If a fresh read recovers and confirms an unadopted Solo chat with no recipients or other loaded person, Tag an agent to send. applies and preserves the draft. The same guard runs at initial resolution and final publication validation.
Verification and UI
At
17139248d904ba5c0ddc1683962f433411820762, only the existing primary settings JSX moved above the follow-up queue, restoring queue/form adjacency without changing visibility or settings behavior. The unchanged queue and Solo-primary journeys pass on Linux in Chromium and Playwright WebKit: 6/6 cases on the PR source, then 6/6 on a verification-only merge with main06d1347dab7bc825312d5f217cc2fd12cfab1de0. The second run exercised the settings-visible layout that failed the 8px spacing assertion in hosted CI. The checks cover both themes, narrow/wide widths, enlarged text, keyboard and pointer behavior. No tests or assertions changed. The configured macOS pre-push hooks passed: types/related tests (589.05s), design guards (7.70s), and Clippy (0.36s). Current hosted CI is still running; native/live delivery remains unverified.Earlier validation evidence and source-fixture screenshots (prior heads)
At
1a991b7715d227fd93c2cb21f88621bb37ee2270, the 32-recipient regression first failed through the real writer. The fix preserves all authored recipients, omits only the implicit primary, and uses the existing post-send notice. All 78 primary-service tests, TypeScript, and changed-file Biome pass on the exact source on Linux. The configured macOS pre-push hooks passed: types/related tests (564.00s), design guards (7.69s), and Clippy (0.31s). Hosted CI subsequently exposed the queue/form spacing regression addressed above; native delivery remains unverified.At
da28afea0aff58988fe7a800e95c9603a922c4b7, regression checks first reproduced the missing MCP mention marker and the settings-confirmation recovery failure. The fixes pass all 220 tests across the three affected services on Linux, plus TypeScript. Changed-file Biome and the configured macOS pre-push hooks passed on the exact committed tree: types/related tests (672.82s), design guards (9.03s), and Clippy (0.27s). The required hosted checks are green; native delivery remains unverified.Merge head
2ffbd0fd3a19b88feef8a36656d685175570b5c0merged main68ebbfeb1b8a565f8a4ed517e94ed63fbcddb418without changing the primary-binding production patch. On that exact tree, all 743 tests across the five affected owner files passed on Linux, along with TypeScript and changed-file Biome. The configured macOS pre-push hooks passed: types/related tests (1166.20s), design guards (7.77s), and Clippy (0.59s). Both sides’ regression assertions are retained. Hosted CI on that head finished with 37 passed checks and one skipped; native delivery remains unverified.On the exact source of
a502894a206198821d577d42476c137cffa0c0b3, all 396 tests across the full composer and Solo channel files and changed-file Biome pass on Linux. The recovered-owner regression holds the final authoritative read: removing the final guard reproduced the missing refusal before the final text correction; the fixed send preserves the draft, while a chat with another loaded person still sends. The frozen install and configured macOS pre-push hooks passed: types/related tests (482.75s), design guards (8.02s), and Clippy (0.38s). Existing Reload primary focus assertions are unchanged; their channel snapshot fixture is now stable for #1423’s subscription. No browser cases or assertions changed in this integration; native delivery remains unverified.The recovered-owner send test uses the real primary resolver and a failed presentation read followed by successful authoritative recovery. It fails before the fix because the existing tag guard is absent. On rebased head
a658fab207c98c668a63647b180e79c43ad5eb8f, both affected unit files pass all 432 tests on Linux, with TypeScript and changed-file Biome passing. The normal macOS pre-push hooks then passed on that head: types and related tests (502.98s), design checks (9.18s), and Clippy (0.89s). The focused bundled README test also passed after a local cache clear. Main’s recipient-replacement tests and the primary-agent tests are both retained. No browser cases or assertions changed; native delivery remains unverified.Source-fixture screenshots (ephemeral identity; not a native capture):
Before:
After adoption (earlier source-fixture revision; current owner controls differ):
The browser fixture uses ephemeral identities and local writers with the real session/composer. It covers an owner send, mention-only save, keyboard focus restoration and narrow-width wrapping. The recipient, revision, revocation and conversion matrices remain in unit tests; no browser cases were removed. Chromium and Playwright WebKit source-fixture checks do not prove native WKWebView delivery, which remains unverified.
Shared owners: Button and Select, with the existing wrapping flex/spacing recipe. Rendered checks exercised light/dark themes at 390px and 1100px widths with 1.4x text, pointer selection, keyboard dismissal/focus and the opt-out. Regression cases failed against the pre-fix behavior, including stale-label adoption, secondary-target preservation and human-send availability. An additional test uses the actual agent relay-session inventory and checks conversion and session disposal. The accepted fixes have deterministic negative-before coverage for guest sends, signed-recipient provenance across adoption, unknown-binding denial after human publication, and cancellation during the last send-time read. Immediate sends cancel on disablement, navigation or unmount; already-queued follow-ups retain their captured lifetime. On
30e73880de0d6980867cbeefbad6e9081d7f7b1b, the three affected unit files passed 312 tests. All four affected browser files passed 38 cases across Chromium and Playwright WebKit. On prior comment-only head2bee5f006442aaa51cfba0fcafc02dcf220069bb, the configured pre-push hook passed TypeScript, design guards and 4,938 related tests; Clippy correctly skipped because no Rust inputs changed. Hosted Rust checks remain required. New regressions failed first against the old code: an unreadable primary no longer drops unrelated catch-up events, a removed secondary cannot start after its final roster read, and canonical authored mentions pass through the real session signing/publication boundary. The last defect caused the six failed browser shards on the prior head: its session validator still accepted only bare two-elementptags. It now accepts canonical four-element mention tags while rejecting malformed variants.The converted-chat recovery regression failed before the fix because presentation skipped adoption discovery. With the fix, both affected unit files pass all 410 tests and TypeScript passes on Blox. The configured pre-push hook passed on
99c77031e459bf358d25e559871f29015cdad020: TypeScript, design guards and 4,939 related tests; Clippy skipped because no Rust inputs changed. Admission rules are unchanged; the missing-binding notice now points at restoring primary labels in channel settings.Earlier admission/discovery regressions failed against the prior code, and the corrected owner files passed 573 tests on a guarded Mac run. The hooks-intact push of rebased head
169b561912d54474becc03606e3cab736697e5a1passed types/related tests (294.78s) and design checks (19.67s). The current change preserves independent watch admission instead of applying primary-recipient checks to every catch-up event; addressed mentions still receive fresh validation. Hosted Rust checks remain required.Explicit background-upload Cancel now reaches primary revalidation and the final publication boundary through the job signal. A held-read regression failed first because the cancelled job still signed; all 344 composer tests pass with the fix, with the draft and attachment restored. Navigation still leaves session-owned uploads running.
The linked-tab WebKit failure was a fixture-ordering defect: unrelated Solo interactions kept the initial Gamma read held beyond its 10-second deadline. The test now proves live delivery during the held read and releases it immediately, before those interactions. All 14 cases in the affected file pass across Chromium and Playwright WebKit. No timeouts or assertions changed.
No browser cases were added or removed in this correction. The shared synthetic-relay fixture explicitly handles the adoption query. Hosted
App.profiletimeout also reproduced on main. The two earlier hosted WebKit failures were the held-pointer history anchor and the linked-tab destination. Each scenario passed ten repetitions on baseline17266ddfand ten on final head2bee5f0(20 cases per snapshot); the cause of the hosted failures remains unproven. No test or assertion was weakened.The pin-only adoption regression failed before the fix: removing the last evidence restored bare-target admission in the same session. All 71 primary tests pass after retaining that veto; a new session still observes a complete owner reset.
Explicit-send regressions hold presentation discovery with unrelated cached labels. Both cases failed first because resolution was skipped; all 346 composer tests pass after the fix. The cases check authored/revision targeting and a visible unavailable-primary notice while the human message still posts. The ordinary untagged pending-send case still passes. No browser cases were added or removed.
Before the #1423 compatibility integration, partial inventory and recovered legacy-candidate regressions failed first, then all 421 tests in the two affected owner files passed. That legacy inference is now removed: unadopted chats preserve authored recipients under #1423. No browser cases were added or removed in that earlier correction.
Maintainer feedback is addressed in
e6ea52ac5504078d3ae22aaffbe2744b891168d0: primary-recipient checks apply to addressed mention turns, while independently configured watches retain attention admission. Real-capability live/catch-up regressions failed first with zero deliveries; Reload primary focus regressions failed first at the focus assertion. A minimal admission-start barrier preserves the existing mention-first invocation order without holding independent lanes through the run lifetime; all six controlled-ordering cases failed before the barrier. The full service and composer files pass all 591 tests (zero skips), with TypeScript, Biome and diff checks passing on the same tree. No browser cases were added or removed in this correction. Keyboard focus coverage here is jsdom/StrictMode, not new native or rendered-host verification.Mac pre-push checks passed on
e6ea52ac: TypeScript and related unit tests (524.42s), design guards (9.10s), and Clippy (0.76s). The fixture-only correction at4e4a70c9also passed the locked install and unchanged hooks: TypeScript/related tests (501.07s), design guards (7.71s), and Clippy (0.24s). Earlier-head hosted CI completed: 31 jobs passed and one was skipped. Only the Chromium/WebKit settings shards and their required aggregate failed, for the then-present upstream-main issue below.The synthetic relay now applies its exact primary-history assertions only to kind 45011. Ordinary current-artifact queries (45010) retain their existing fallback. The previous Chromium and WebKit failures exposed this fixture routing defect; no assertions or browser cases were removed. Local browser replay was not admitted under host load. Hosted CI now passes all previously fixture-failing cases in both engines, including both browser flake-check jobs. No browser cases or assertions changed.
The earlier upstream settings-test failure was fixed by #1488; main CI on base
8466aeaa0passed. The earlier compatibility integration was rebased onto43da0493band preserved #1423’s assertions.Hosted timing samples for the fixture correction (not a controlled performance comparison; merge refs differ):
e6ea52ac4e4a70c9These used hosted Linux, Playwright 1.63.0 Noble, two workers, and
playwright test --config tests/browser/playwright.ci.config.mjs --project <engine> --no-deps --shard=<shard> --reporter=list,json. Setup is first job log to the test-run announcement; test time is the runner-reported elapsed time. The checked merge refs were90a2dd88andfbe560f0, respectively.— Posted by Codex (gpt-6.1-sol) on behalf of kylez