docs: address review comments on product plan and doc index - #114
Merged
Merged
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Two unresolved moderate documentation issues remain in and around docs/PRODUCT_PLAN.md.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Documentation-only update refocusing the product plan on post-0.7.0 work and synchronizing documentation indexes.
Changes:
- Adds lifecycle priorities and acceptance gates.
- Clarifies endpoint conformance and architecture references.
- Updates documentation indexes and changeset metadata.
File summaries
| File | Summary |
|---|---|
docs/README.md |
Updates the Product Plan description. |
docs/PRODUCT_PLAN.md |
Updates priorities, gates, prerequisites, and architecture anchors. Moderate findings remain regarding failed-renderer recovery coverage (2 votes) and outdated docs/BACKLOG.md ownership text (1 vote). |
AGENTS.md |
Synchronizes the documentation map. |
.changeset/docs-product-plan-compact.md |
Records the documentation-only changes. |
Review details
Suppressed comments (4)
docs/PRODUCT_PLAN.md:38
- The gate's final clause still treats
tool-capableas the source of truth, butsupportsToolCallingis only catalog-declared metadata and the roadmap explicitly distinguishes it from Flint-verified behavior. Require an actual request with tool definitions that yields validtool_calls, and report the catalog declaration separately, so this gate cannot pass based only on metadata.
- **Endpoint conformance gate:** User-facing self-test verifies `/v1/models` envelope, model ID reuse in chat completions, streaming termination with `[DONE]`, disconnect cancellation, and verified `tool_calls` output for tool-capable models.
docs/PRODUCT_PLAN.md:42
- The telemetry gate omits load time even though the workstream's priority explicitly promises it. Without it, a release can satisfy this gate while leaving one of the promised telemetry fields unimplemented; include load time in the gate.
- **Inference telemetry gate:** Diagnostics and UI display accurate TTFT, prompt tokens/sec, decode tokens/sec, and provider tags without ambiguous aggregated rates.
docs/PRODUCT_PLAN.md:78
- The workstream table's endpoint target scope still lists only envelope, streaming, and cancellation, while the new acceptance gate also requires model-ID reuse and tool-call conformance. That leaves this summary out of sync with the plan's stated conformance scope; update the target scope to include those checks.
| **Endpoint behavioral self-test** | Diagnostic tool in UI testing OpenAI envelope, streaming chunk validity, and cancellation. | Gateway proxy & initial normalization (Phase 1B delivered; broader conformance in progress). |
docs/PRODUCT_PLAN.md:35
- After the finding register was removed,
docs/BACKLOG.md:11-12still says PRODUCT_PLAN owns the review findings. That contradicts the new ownership model (PRODUCT_PLAN now has sequencing/gates and BACKLOG is open-work only); update that statement as part of this documentation move.
## Acceptance gates for post-0.7.0 workstreams
Each post-0.7.0 workstream must satisfy these concrete acceptance criteria before shipping:
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Several acceptance gates remain incomplete or inconsistent with their stated workstreams, and the changeset is overly detailed.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 4
- Review effort level: Balanced
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Cross-document contradictions and incomplete acceptance criteria remain.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
docs/BACKLOG.md:14
- This still presents “Fix data loss” as current open work, but this file declares that it contains open work only and
CHANGELOG.md:5,13records conversation data integrity as delivered in 0.7.0. The revised product-plan priorities also contain no data-loss workstream. Replace the stale imperative with the remaining runtime-hardening scope.
docs/PRODUCT_PLAN.md:38
- This does not cover all work in the endpoint/agent workstream as claimed by the PR: it omits the
usagereconciliation check required byRELEASE_ROADMAP.md:42-45and the version-pinned agent recipes listed at line 21 andRELEASE_ROADMAP.md:53-55. As written, the workstream can satisfy this gate while those explicit deliverables remain unverified; include them here or add a separate integration-recipes gate.
- **Endpoint conformance gate:** User-facing self-test verifies `/v1/models` envelope, model ID reuse in chat completions, streaming termination with `[DONE]`, disconnect cancellation, and verified generation of valid `tool_calls` structures on actual tool-definition prompts (distinguishing verified behavior from catalog-declared `supportsToolCalling` metadata).
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Balanced
|
|
||
| ## Decisions | ||
|
|
||
| <a id="workstream-c-thin-native-ownership----expedited"></a> |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Several new acceptance criteria are ambiguous or not objectively achievable as written.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
docs/PRODUCT_PLAN.md:40
- “Without increasing application bundle size” makes this gate effectively unsatisfiable: any added decoder code can increase the artifact, and the capability table explicitly permits a lightweight WASM decoder. Define a measurable maximum size increase (or say “without a heavyweight decoder dependency”) so the gate agrees with the planned implementation and can be evaluated objectively.
- **Audio transcoding gate:** Client-side WebM/Opus and MP3 decoding converts to 16 kHz mono PCM WAV before ingestion without increasing application bundle size.
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Balanced
| - **Native supervision (Phase 2):** Rust-owned sidecar child process, exclusive stdio JSON-lines transport bridge, bounded write queues with backpressure, native single-instance enforcement, and output-gate fencing on exit. | ||
| - **Packaging & dependencies:** Pinned Node 22 runtime bundled as an external binary, verified native Foundry core assets, and macOS Gatekeeper installation handling. | ||
| - **Installed-path & lifecycle gate:** Native system tray Open/Quit and macOS Dock reopen restore the main window across packaged Windows and macOS builds; single-instance launch refocuses existing instance; process termination cleanly tears down the sidecar without orphaned processes; and recovery controls operate independently if the renderer webview fails. | ||
| - **Endpoint & agent integration gate:** User-facing self-test verifies `/v1/models` envelope, model ID reuse in chat completions, streaming termination with `[DONE]`, disconnect cancellation, `usage` token count reconciliation with output, verified generation of valid `tool_calls` structures on actual tool-definition prompts (distinguishing verified behavior from catalog-declared `supportsToolCalling` metadata), and verified integration recipes pinned to tested client versions (Continue, Cline, OpenClaw). |
Clarify that audio transcoding uses Web Audio or lightweight decoders without heavyweight dependencies rather than zero bundle size increase. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Two acceptance gates are incomplete or describe incorrect lifecycle behavior.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
docs/PRODUCT_PLAN.md:37
- This criterion currently says tray Quit restores the window, which conflicts with the lifecycle behavior being specified. Separate the Open/reopen restoration checks from the requirement that Quit exits cleanly so the gate cannot be interpreted or tested incorrectly.
- **Installed-path & lifecycle gate:** Native system tray Open/Quit and macOS Dock reopen restore the main window across packaged Windows and macOS builds; single-instance launch refocuses existing instance; process termination cleanly tears down the sidecar without orphaned processes; and recovery controls operate independently if the renderer webview fails.
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Balanced
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
The restored anchor exposes a contradictory Node-process claim in the linked design specification.
Review details
Suppressed comments (1)
docs/PRODUCT_PLAN.md:47
- Restoring this anchor makes
FLINT_DESIGN_SPEC.md:162resolve, but that link still claims the native supervisor “would remove the Node process.” The target section instead says Rust owns one runtime child and retains the existing sidecar, which matchessrc-tauri/src/runtime_manager.rs:123-142spawning bundled/path Node. Please update the linked design-spec sentence as part of this synchronization so the repaired cross-reference does not lead from a contradictory architecture claim.
<a id="workstream-c-thin-native-ownership----expedited"></a>
### Native ownership and runtime architecture
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Balanced
…or reference Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The design specification retains a stale “Workstream C” label for a renamed product-plan section.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Balanced
|
|
||
| - Move selected hot paths or packaging concerns to Rust when reliability/footprint benefits are clear | ||
| - A native Rust supervisor (see [PRODUCT_PLAN.md Workstream C](./docs/PRODUCT_PLAN.md#workstream-c-thin-native-ownership----expedited)) would remove the Node *process*, not just PATH dependence — bundled Node already removed the PATH requirement | ||
| - A native Rust supervisor (see [PRODUCT_PLAN.md Workstream C](./docs/PRODUCT_PLAN.md#workstream-c-thin-native-ownership----expedited)) supervises the runtime sidecar child, establishes exclusive stdio transport, and enforces single-instance lifecycle recovery |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses review feedback on
docs/PRODUCT_PLAN.mdand related index documents:CHANGELOG.md).FLINT_DESIGN_SPEC.mdlink to native supervisor decisions.docs/README.mdandAGENTS.md.