Replace capture profiles with explicit capabilities - #58
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5b862a2615
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| TugboatViewportSemanticScrollContext? scrollContext, | ||
| }) { | ||
| if (config.profile != TugboatCaptureProfile.exploration) return; | ||
| if (!config.sceneInventoryEmissionEnabled) return; |
There was a problem hiding this comment.
When a host enables emitViewportSemanticMap: true with a supported full semantic mode but leaves the separate emitSceneInventory capability false, this early return prevents the inventory needed to build a map from being created, so _viewportSemantics.maybeEmit is never reached and neither viewport-map nor scroll-semantic events are emitted. Build the internal inventory when either capability needs it, while gating only the scene_inventory event on emitSceneInventory.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1788cc5. Semantic-map capability now builds the internal inventory independently. Raw scene_inventory emission still requires its own capability. Added a regression test for semantic-map-only configuration.
There was a problem hiding this comment.
🟡 Changes recommended
The lifecycle state tracking can remain “dormant” during config-enabled capture, and there are a couple of correctness/documentation fixes needed to avoid silent misconfiguration and inconsistent guidance.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR migrates the Flutter Tugboat SDK away from capture profiles to a single privacy-safe capture behavior controlled by TugboatReplayConfig.enabled, with additive opt-in capabilities for higher-sensitivity evidence (scene inventory, semantic maps, diagnostics, action context). It also bumps tugboat and tugboat_dio to 0.9.0 and updates tests/docs to match the new API surface.
Changes:
- Remove
TugboatCaptureProfileand replace lifecycle control withenabledplusactivate()/deactivate(). - Add explicit capability flags (
emitSceneInventory,emitViewportSemanticMap,emitCaptureDiagnostics,acceptActionContext) and update controller behavior accordingly. - Update documentation, changelogs, and tests; bump package versions to
0.9.0.
File summaries
| File | Description |
|---|---|
| sdks/flutter/packages/tugboat/test/viewport_semantic_policy_test.dart | Updates semantic policy tests to use explicit semantic-map capability instead of profiles. |
| sdks/flutter/packages/tugboat/test/viewport_semantic_map_test.dart | Updates semantic-map session tests to the new enabled/capabilities config. |
| sdks/flutter/packages/tugboat/test/tugboat_replay_test.dart | Updates replay tests for lifecycle, masking defaults, and action-context API rename; adds capability coverage. |
| sdks/flutter/packages/tugboat/test/scroll_playground_live_test.dart | Updates end-to-end scroll playground config to the new capability flags. |
| sdks/flutter/packages/tugboat/test/scroll_attribution_test.dart | Updates scroll attribution tests to use enabled/capabilities. |
| sdks/flutter/packages/tugboat/test/scene_inventory_test.dart | Updates scene-inventory tests to use enabled/capabilities. |
| sdks/flutter/packages/tugboat/test/replay/tugboat_health_test.dart | Updates health snapshot tests to reflect removal of profile fields. |
| sdks/flutter/packages/tugboat/test/replay/tap_coordinate_transform_test.dart | Updates coordinate transform test config to enabled/capabilities. |
| sdks/flutter/packages/tugboat/test/replay/route_overlay_identity_test.dart | Updates overlay identity fixture config to enabled/capabilities. |
| sdks/flutter/packages/tugboat/test/replay/replay_overlay_nested_navigation_matrix_test.dart | Updates nested navigation fixture config to enabled/capabilities. |
| sdks/flutter/packages/tugboat/test/replay/pan_zoom_gesture_test.dart | Updates harness usage to new config fields (e.g., disabling semantic-map emission). |
| sdks/flutter/packages/tugboat/test/replay/modal_capture_visual_test.dart | Updates modal capture visual test config to enabled/capabilities. |
| sdks/flutter/packages/tugboat/test/replay/capture_diagnostics_test.dart | Updates diagnostics tests to match opt-in diagnostic event emission behavior. |
| sdks/flutter/packages/tugboat/test/locale_evidence_test.dart | Updates locale evidence tests to enabled/capabilities. |
| sdks/flutter/packages/tugboat/test/integration/release_compatibility_matrix_test.dart | Updates compatibility matrix integration tests to enabled/capabilities and new activate signature. |
| sdks/flutter/packages/tugboat/test/helpers/replay_coherence_harness.dart | Refactors harness construction to use enabled + selected capabilities. |
| sdks/flutter/packages/tugboat/test/external_event_and_network_test.dart | Updates external event/network tests for mode-free parameter policy and action-context rename. |
| sdks/flutter/packages/tugboat/test/exploration_pre_tap_test.dart | Updates pre-tap evidence tests to the new inventory/semantic-map capabilities model. |
| sdks/flutter/packages/tugboat/README.md | Documents 0.9.0 API shift (enabled + additive capabilities) and updates version pins. |
| sdks/flutter/packages/tugboat/pubspec.yaml | Bumps tugboat package version to 0.9.0. |
| sdks/flutter/packages/tugboat/lib/tugboat.dart | Removes export of TugboatCaptureProfile from the public barrel. |
| sdks/flutter/packages/tugboat/lib/src/tugboat.dart | Updates public APIs (activate, health snapshot fields, eventHook default policy) and gate behavior to enabled-based lifecycle. |
| sdks/flutter/packages/tugboat/lib/src/sinks/capture_sink.dart | Removes capture-profile name from sink session context. |
| sdks/flutter/packages/tugboat/lib/src/sdk_version.dart | Updates embedded SDK version constant to 0.9.0. |
| sdks/flutter/packages/tugboat/lib/src/replay_config.dart | Introduces enabled and capability flags; updates semantic-policy resolver and masking defaults. |
| sdks/flutter/packages/tugboat/lib/src/lifecycle.dart | Replaces profile-based lifecycle override with boolean capture override. |
| sdks/flutter/packages/tugboat/lib/src/health.dart | Removes profile field from SDK health snapshot and JSON. |
| sdks/flutter/packages/tugboat/lib/src/external_event.dart | Removes production-scoped parameter policy mode; simplifies allow-all behavior and docs. |
| sdks/flutter/packages/tugboat/lib/src/evidence_recorder.dart | Removes profile coupling and updates default parameter policy. |
| sdks/flutter/packages/tugboat/lib/src/controller.dart | Gates diagnostics/action-context/scene inventory/semantic maps via capabilities; removes profile conditionals. |
| sdks/flutter/packages/tugboat/lib/src/capture_profile.dart | Deletes the old capture profile enum. |
| sdks/flutter/packages/tugboat/example/pubspec.yaml | Bumps example dependency pin to ^0.9.0. |
| sdks/flutter/packages/tugboat/CHANGELOG.md | Adds 0.9.0 changelog with migration guidance. |
| sdks/flutter/packages/tugboat_dio/test/tugboat_dio_interceptor_test.dart | Updates interceptor test config to enabled lifecycle. |
| sdks/flutter/packages/tugboat_dio/pubspec.yaml | Bumps tugboat_dio to 0.9.0 and updates dependency on tugboat. |
| sdks/flutter/packages/tugboat_dio/CHANGELOG.md | Adds 0.9.0 compatibility release note. |
| docs/releases/compatibility.md | Adds 0.9.0 row and removes “planned” entry. |
| docs/integration/collector.md | Updates integration guidance/examples to enabled + capabilities. |
| docs/design/capture-and-fingerprint.md | Updates design notes to describe lifecycle + capability model and masking/semantics behavior. |
Review details
- Files reviewed: 39/39 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
@codex review |
There was a problem hiding this comment.
🟡 Changes recommended
There are confirmed lifecycle and semantics-policy issues (deactivate no-op guard and persistent semantics handle gating) that can cause incorrect lifecycle transitions and unnecessary runtime overhead.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
sdks/flutter/packages/tugboat/lib/src/lifecycle.dart:53
- deactivate() is intended to be a safe no-op when already dormant with no runtime override. Because the guard only returns when _captureOverride == false (not null), calling deactivate() in the initial/default state (_captureOverride == null) will still bump requestEpoch and transition to stopping, causing unnecessary lifecycle churn and notifications.
- Files reviewed: 39/39 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 01472f13a3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (oldWidget.config.enabled != widget.config.enabled) { | ||
| _syncCaptureFlag(); | ||
| } |
There was a problem hiding this comment.
When an already-enabled wrapper rebuilds to grant or revoke a capability such as emitSceneInventory, this condition does nothing because enabled remains true. The _TugboatReplayRoot retains the same key, and its controller was initialized once with the old config, so newly granted capabilities never emit and revoked capabilities can continue emitting until capture is explicitly torn down. Include the capability fields in the remount/update decision, particularly so a runtime authorization revocation takes effect.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in bb01903. A capability grant or revocation now advances the session epoch and remounts the capture root. A regression test verifies both grant and revoke changes while capture stays enabled.
|
The suppressed Copilot lifecycle comment is also fixed in bb01903. deactivate() now stays a no-op in the initial dormant state. A regression test covers this case. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb01903d36
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (config.sceneInventoryEmissionEnabled && isPrimaryPointer) { | ||
| _captureExplorationPreTapEvidence(tx); |
There was a problem hiding this comment.
When emitSceneInventory is enabled but emitCaptureDiagnostics remains false, every primary pointer-down still enters _captureExplorationPreTapEvidence, which unconditionally records an exploration_pre_tap_diagnostic event. This leaks diagnostic-stream events through the inventory capability even though the new capabilities are documented as independent and diagnostic evidence defaults off; gate _recordExplorationPreTapDiagnostic on emitCaptureDiagnostics while retaining the inventory work.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 517aace. Pre-tap inventory work remains active, but diagnostic event emission now requires emitCaptureDiagnostics. Added a regression test for inventory enabled with diagnostics disabled.
There was a problem hiding this comment.
🔵 Needs a closer look
The controller currently triggers viewport semantic-map build work even when semantic-map emission is disabled, which is a measurable avoidable performance cost in the new capability model.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
sdks/flutter/packages/tugboat/lib/src/controller.dart:5655
- _emitSceneInventory() will call _viewportSemantics.maybeEmit() whenever emitViewportSemanticMap is true (default), even when the resolved config.semanticMapEmissionEnabled is false. Since viewport_semantic_session builds maps before checking emitEvents, this can do unnecessary work for inventory-only capture or tapResolutionOnly mode. Consider gating the maybeEmit() call on config.semanticMapEmissionEnabled as well.
}
if (emitViewportSemanticMap) {
_viewportSemantics.maybeEmit(
inventory,
resolver: _anchorResolver,
scrollContext: scrollContext,
);
}
- Files reviewed: 39/39 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
@codex review |
|
The suppressed Copilot performance comment is fixed in c039a27. The controller now calls semantic-map emission only when the resolved semanticMapEmissionEnabled policy is true. The default-mode test now enables the raw capability and confirms no map emission. |
|
@codex review |
There was a problem hiding this comment.
🟡 Changes recommended
handleExplorationControl() in controller.dart has switch-case fallthrough (missing break/return), which is a Dart compile error and would mis-handle control messages.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 39/39 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c039a27b19
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (oldWidget.config.enabled != widget.config.enabled) { | ||
| _syncCaptureFlag(); | ||
| } else if (_captureMounted && |
There was a problem hiding this comment.
When capture remains active because TugboatReplay.activate has set the runtime override, rebuilding from enabled: false to enabled: true while also granting or revoking a capability enters this first branch. _syncCaptureFlag() keeps the existing mount because capture is already requested, and the else if then skips the capability remount, so the controller continues using its old capability values. Check _capabilitiesChanged after synchronizing whenever the mount survives the lifecycle change.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in cc2ec79. Lifecycle synchronization and capability-change detection now run independently. A regression test activates runtime capture, then changes enabled and an inventory grant in one rebuild, and confirms controller remount with the new grant.
There was a problem hiding this comment.
🔵 Needs a closer look
This is a broad, breaking public SDK refactor (lifecycle + capabilities + defaults) that impacts many core code paths and warrants final human review despite passing tests.
Review details
- Files reviewed: 39/39 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
@codex review |
There was a problem hiding this comment.
🔵 Needs a closer look
It is a breaking, cross-cutting SDK lifecycle/API refactor that touches core capture behavior, public API surface, and multiple documentation and test suites.
Review details
- Files reviewed: 39/39 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Summary
enabled,activate, anddeactivatetugboatandtugboat_dioto 0.9.0Validation
bash tool/ci/run-flutter-tests.sh tugboat(399 passed)bash tool/ci/run-flutter-tests.sh tugboat_dio(20 passed)dart analyze .(exit 0; one pre-existing info)bash tool/ci/verify-native-capture-pigeon.shbash tool/ci/check-license.shgit diff --checkCompatibility
This is a breaking SDK API change. The package versions move to 0.9.0. Historical exploration transport and wire field names remain unchanged for schema compatibility.