Repository navigation
Conversation
Size Report
Startup median (7 runs, lower is better):
|
aedf9f5 to
0315a02
Compare
|
At 0315a02, one defect failed CI, and the design needs a decision before merge. The newer head 8180363 changes only packages/replay/src/plugin.ts:4 uses Not blocking, and you can take or leave these: (1) Could this land as one layer instead of a standalone package and an unconsumed resolver? That layer would declare the native execution port in I did not run tests, typecheck, or build and pack |
8180363 to
9f1fa69
Compare
thymikee
changed the title
9f1fa69 to
25e621d
Compare
25e621d to
499d4bc
Compare
|
The code at 499d4bc is ready; I found no problems that need a change. Not blocking, and fine to take or leave: |
499d4bc to
3e5030d
Compare
|
Current registry head is I am retaining the small existing registry implementation rather than adding a shared create-once abstraction solely to remove similar lines. The independent smaller-design assessment accepted the three execution entrypoints and accounted for the +34.7 kB package growth (mostly declarations); it did not establish a performance win. The native ADR0034 amendment remains Proposed, pending maintainer acceptance. Exact-head affected admission passed: 42 passed / 25 skipped, exactCommit=true, checkoutChanged=false. All 19 hosted checks passed. Later source/help fixes are owned by the top routing layer, so this head has not been churned. No registry/source readiness statement here clears C3, live/platform, ADR or npm placeholder/trusted-publisher prerequisites. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 11 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="docs/adr/0034-provider-plugin-registry.md">
<violation number="1" location="docs/adr/0034-provider-plugin-registry.md:60">
P2: This adds an unaccepted proposal to an ADR marked `accepted`, contrary to the repository’s ADR format. Keep the amendment in #3871 until acceptance, or merge it only after updating the ADR to the accepted decision.</violation>
<violation number="2" location="docs/adr/0034-provider-plugin-registry.md:74">
P2: This overstates the native contract: replay execution has dispatch and progress ports, but no cancellation port, while suite cancellation is a separate dependency. Document the ports per method so plugin authors do not implement a nonexistent cancellation input.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
|
|
||
| Contracts own the inspection, step, suite and factory types. | ||
| Native `create()` receives no format-backend host. | ||
| Each execution call receives explicit host ports for dispatch, progress and cancellation. |
There was a problem hiding this comment.
P2: This overstates the native contract: replay execution has dispatch and progress ports, but no cancellation port, while suite cancellation is a separate dependency. Document the ports per method so plugin authors do not implement a nonexistent cancellation input.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At docs/adr/0034-provider-plugin-registry.md, line 74:
<comment>This overstates the native contract: replay execution has dispatch and progress ports, but no cancellation port, while suite cancellation is a separate dependency. Document the ports per method so plugin authors do not implement a nonexistent cancellation input.</comment>
<file context>
@@ -55,6 +55,33 @@ had to fake the store to load a plugin from its own dependencies (#3747).
+
+Contracts own the inspection, step, suite and factory types.
+Native `create()` receives no format-backend host.
+Each execution call receives explicit host ports for dispatch, progress and cancellation.
+A format factory still receives `ReplayBackendHost` for source collection, execution and export.
+Both factories use the registry's claims, inventory, lazy creation, failure isolation and disposal.
</file context>
| Each execution call receives explicit host ports for dispatch, progress and cancellation. | |
| `runAdReplay` receives explicit dispatch and progress ports; suite execution receives explicit progress and cancellation ports, with dispatch supplied through its `runReplay` dependency. |
There was a problem hiding this comment.
Accepted: the sentence overstates AdReplayStepRuntime. runAdReplay receives dispatch and progress inputs, with no cancellation port; runReplayTestSuite receives its explicit progress/cancellation dependencies and dispatch through runReplay. The corrected proposal is preserved at #3871 (comment). ADR removal is queued with the stack update. Leaving this thread open until the source correction is published; no nonexistent cancellation API is being added.
|
|
||
| ## Proposed 0.22 amendment: native execution on the replay backend | ||
|
|
||
| This amendment is proposed for #3871 and awaits maintainer acceptance. |
There was a problem hiding this comment.
P2: This adds an unaccepted proposal to an ADR marked accepted, contrary to the repository’s ADR format. Keep the amendment in #3871 until acceptance, or merge it only after updating the ADR to the accepted decision.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At docs/adr/0034-provider-plugin-registry.md, line 60:
<comment>This adds an unaccepted proposal to an ADR marked `accepted`, contrary to the repository’s ADR format. Keep the amendment in #3871 until acceptance, or merge it only after updating the ADR to the accepted decision.</comment>
<file context>
@@ -55,6 +55,33 @@ had to fake the store to load a plugin from its own dependencies (#3747).
+## Proposed 0.22 amendment: native execution on the replay backend
+
+This amendment is proposed for #3871 and awaits maintainer acceptance.
+The existing accepted provider contract and lifecycle rules retain their status.
+The draft API adds native execution to `kind: 'replay-backend'` with backend `ad`.
</file context>
There was a problem hiding this comment.
Accepted under docs/adr/FORMAT.md: an unaccepted proposal belongs in #3871. I preserved the full proposal, with corrected per-method port wording, at #3871 (comment). The queued patch removes only the proposed section from ADR0034; its accepted decisions and accepted status remain unchanged. This does not self-approve the native extension or clear its integration blocker. Leaving this thread open until that source deletion is published.
|
Found 1 test failure on Blacksmith runners: Failure
|
![Fix with [code]smith](https://pr-comments-assets.blacksmith.sh/codesmith/fix-with-codesmith-light-3x.png)
Apex Sentinel · ✅ ApproveSince the last review: 1 new · 0 still open · 0 no longer raised 1 finding(s): 1 minor. 🟡 Minor (1)
1 finding below 💬 Talking to SentinelIn a finding's thread, reply with:
On this conversation, comment Earlier reviewsReview of
|
|
Confirmed at The lower Maestro-layer Apple setup correction is already queued in the coordinated stack refresh, so it will reach this PR too. It installs the built plugin after the shared build under a persistent job-scoped plugin home. No additional engine change or CI retry is needed for this diagnostic. Fresh hosted iOS success remains required after publication. |
3e5030d to
2c5f4d1
Compare
2c5f4d1 to
8940075
Compare
![apex-sentinel-staging[bot]](https://avatars.githubusercontent.com/in/5196457?s=60&v=4){kind=small}
There was a problem hiding this comment.
Apex Sentinel: 1 new finding on 8940075. The verdict and the full list are in the summary comment.
| resolveReplayExecution: async (packageName) => { | ||
| if (closed) | ||
| throw new AppError('COMMAND_FAILED', 'Plugin registry is closed', { | ||
| reason: 'plugin-registry-closed', | ||
| }); | ||
| const slot = requireReplaySlot(resolved, NATIVE_REPLAY_BACKEND, packageName); | ||
| if (slot.failure !== undefined) throw slot.failure; | ||
| const execution = slot.replaySetup?.execution; | ||
| if (!execution) throw invalidPlugin('Native replay plugin must provide execution.create()'); | ||
| return await (slot.executionLoading ??= Promise.resolve().then(async () => { | ||
| try { | ||
| return validateReplayExecution(await execution.create()); | ||
| } catch (error) { | ||
| slot.failure = asPluginFailure(error, 'plugin_load_failed'); | ||
| inventory.splice(0, inventory.length, ...resolved.inventory()); | ||
| throw slot.failure; | ||
| } | ||
| })); | ||
| }, |
There was a problem hiding this comment.
🆕 🟡 Minor · resolveReplayExecution can pick another backend's slot and report invalid_plugin, not plugin-not-installed
A native request with ad not installed but packageName matching an installed slot (e.g. @agent-device/maestro) gets invalid_plugin instead of plugin-not-installed because requireReplaySlot falls back to a package-name match, and slot.replaySetup?.execution is undefined on non-ad backends. The consumer in replay-backend-registry.ts that maps plugin-not-installed into an install hint never triggers.
| resolveReplayExecution: async (packageName) => { | |
| if (closed) | |
| throw new AppError('COMMAND_FAILED', 'Plugin registry is closed', { | |
| reason: 'plugin-registry-closed', | |
| }); | |
| const slot = requireReplaySlot(resolved, NATIVE_REPLAY_BACKEND, packageName); | |
| if (slot.failure !== undefined) throw slot.failure; | |
| const execution = slot.replaySetup?.execution; | |
| if (!execution) throw invalidPlugin('Native replay plugin must provide execution.create()'); | |
| return await (slot.executionLoading ??= Promise.resolve().then(async () => { | |
| try { | |
| return validateReplayExecution(await execution.create()); | |
| } catch (error) { | |
| slot.failure = asPluginFailure(error, 'plugin_load_failed'); | |
| inventory.splice(0, inventory.length, ...resolved.inventory()); | |
| throw slot.failure; | |
| } | |
| })); | |
| }, | |
| resolveReplayExecution: async (packageName) => { | |
| if (closed) | |
| throw new AppError('COMMAND_FAILED', 'Plugin registry is closed', { | |
| reason: 'plugin-registry-closed', | |
| }); | |
| const slot = requireReplaySlot(resolved, NATIVE_REPLAY_BACKEND, packageName); | |
| if (slot.failure !== undefined) throw slot.failure; | |
| const execution = slot.backend === NATIVE_REPLAY_BACKEND ? slot.replaySetup?.execution : undefined; | |
| if (!execution) throw invalidPlugin('Native replay plugin must provide execution.create()'); |
Details
Also raised here
Also raised at this location by persona maintainability: resolveReplayExecution's error is wrong when a slot is matched by package name — requireReplaySlot finds a slot by backend id first and then by packageName, so resolveReplayExecution('some-provider') can land on a slot whose backend is not ad — the NATIVE_REPLAY_BACKEND argument only names the preferred match, it is never asserted. That slot's setup holds replayBackend, the slot.replaySetup?.execution lookup misses, and the caller gets invalidPlugin('Native replay plugin must provide execution.create()') for a plugin that correctly supplied replayBackend.create(host) — while validateReplayEngineSetup simultaneously rejects any native setup that carries both fields. Either restrict the native lookup to slots whose backend is NATIVE_REPLAY_BACKEND (and report plugin-not-installed otherwise) or say in the error that the resolved slot is not the native backend.
Also raised at this location by persona senior-developer: requireReplaySlot package fallback returns a foreign-backend slot, wrong error reason — With only @agent-device/maestro installed, registry.resolveReplayExecution('@agent-device/maestro') finds the maestro slot through the candidate.package fallback in requireReplaySlot and throws invalid_plugin ('Native replay plugin must provide execution.create()') even though the native plugin is simply absent. The caller should get reason 'plugin-not-installed' so it can render the install hint the way replay-backend-registry.missingReplayBackend does; today the reason says the installed plugin is malformed. requireReplaySlot matches on packageName without checking slot.backend === id, so a provider-plugin slot or any other backend's slot can be returned for a native request.
persona: javascript, maintainability, senior-developer
Useful? Reply @apex-sentinel-staging like, or @apex-sentinel-staging dislike <why> if it is not.
Reply @apex-sentinel-staging note ... to add context, or @apex-sentinel-staging pivot ... if it is wrong — I will re-check the cited code.
There was a problem hiding this comment.
Valid at published 894007501dd05245beb766dc36c4415e0f5c065a: package-name fallback can select an active foreign backend when ad is absent. The shared-owner fix is local in 19855593dd57088c277897418d100813484dbc27: backend identity stays authoritative; package fallback is allowed only for an unidentified failed candidate, preserving typed broken-package failures. The suggested execution-only check would still produce invalid_plugin.
The isolated unchanged-test regression is now proven: published registry bytes failed with invalid_plugin where native plugin-not-installed was required; fixed bytes passed. All 15 focused registry cases, typecheck, lint, full format and layering passed. The controls confirm no foreign Maestro factory call, successful own-Maestro resolution, and retained plugin_load_failed for an unreadable selected replay package. These are unit-fixture results, not live execution. The fix is also in review-only c9fc5f653aaa730d6793351b63b40e1765c1f821, preserving that layer’s stale-selection guards; its independent source recheck completed with no findings. Publication remains pending, and this thread stays open until reviewed source is published and verified. The repeated notification requires no second fix or duplicate tests.
|
I reviewed 8940075 and found no code problems. The earlier review of 3e5030d (#4535 (comment)) found the code clean, and the code commits are unchanged since then. The new commit 8940075 only reverts the ADR changes, and ADR 0034 now matches base. Hosted CI is unclear. Coverage failed and ten jobs were cancelled, which fits a superseding push. I could not read the Coverage failure, so I cannot say whether it relates to this PR. I also did not rerun tests locally. Please rerun CI on 8940075 and check the Coverage result. On the open threads: the minor thread about the package-name fallback in the registry still holds (#4535 (comment)). It is not urgent, because Before merge, CI must pass on 8940075 and the two moot threads must be resolved. The maintainer's native-contract acceptance, which the PR names as a merge condition, is still needed. I could not confirm the preserved proposal text in #3871 beyond your reply. |
thymikee
added
the
ready-for-human
Summary
Advance #3871: route native replay execution through the existing managed registry with typed compatibility and stale-selection errors. Keep the proposed native contract in #3871 until accepted.
10 files; 316 gross lines against #4534. The replay package remains private and unpublished.
Validation
Published
894007501dd05245beb766dc36c4415e0f5c065a, base8a215d81e08818cffb02853b13b5b027ef6e94fa. Prior focused results and source approvals retain their named SHA scope. New-head exact affected validation remains pending.Coverage failed: 18 assertions in two host-process fixture files failed; 20,194 tests passed and 70 skipped. The changed-line gate did not run. GitHub tested synthetic merge
4b55d9f8intof9b92418. The process owner and both failing test files are byte-identical across that merge, its base, this head and #4534. Linux/procidentity observations displaced mockedpsresponses. No unchanged-base Linux execution has established causality; this remains blocked, not infrastructure-cleared.Local slot-identity fix
19855593passed isolated red/green and 15 focused cases. Combined review contextc9fc5f65passed 27 focused cases; independent Apex source recheck found no regressions atc9. Neither candidate is published. Size: +34.4 kB unpacked/+9.7 kB download; version startup 19.5→30.0 ms has unresolved cause.Merge blockers remain C3 integration, native-contract acceptance, npm placeholder/trusted publisher setup, and native/platform/remote verification. Ready for review; no merge clearance.