Opt-in Canvas publish via external canvas-pp-cli (PRD §17.5) - #11
Open
johnnyrobot wants to merge 2 commits into
Open
Opt-in Canvas publish via external canvas-pp-cli (PRD §17.5)#11johnnyrobot wants to merge 2 commits into
johnnyrobot wants to merge 2 commits into
Conversation
…s-pp-cli Implements the approved PRD §17.5 decision: Canvas stays read-only by default and the in-app client stays GET-only by construction; publishing a remediated page is an explicit opt-in that shells out to the separately installed, separately authenticated canvas-pp-cli. Guardrails, all enforced: - publish path is invisible unless the CLI binary is detected AND the persisted 'Allow publishing to Canvas' toggle (default off) is on - only Canvas-imported pages can publish (pasted HTML has no target) - the runtime re-runs the accessibility gate on the exact HTML at publish time - a withheld badge refuses the publish (test-asserted) - host-match preflight via 'canvas-pp-cli doctor': the CLI's configured Canvas must equal the host the page was imported from - two-step per-page confirm on the review panel, diff on screen - every publish is recorded in the new canvas_publishes audit table (course, page, SHA-256 of pushed HTML, timestamp) - schema v4 Transport discipline mirrors src/catalog/client.ts: arg-array spawn (never a shell), page HTML over stdin (never argv), validated ids, typed PublishError, timeout-bounded. AppApi grows by three methods (canvasPublishStatus / setCanvasPublishEnabled / publishCanvasPage), implemented across all five implementers + IPC transport per the growth law. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds an opt-in Canvas publishing flow using ChangesCanvas publishing
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Reviewer
participant Renderer
participant IPC
participant RuntimeAppApi
participant CanvasPublisher
participant CanvasCLI
Reviewer->>Renderer: Enable Canvas publishing
Renderer->>IPC: setCanvasPublishEnabled(true)
IPC->>RuntimeAppApi: Persist enablement
Reviewer->>Renderer: Confirm page publish
Renderer->>IPC: publishCanvasPage(baseUrl, courseId, pageId, html)
IPC->>RuntimeAppApi: Re-audit HTML and publish
RuntimeAppApi->>CanvasPublisher: publishPage(...)
CanvasPublisher->>CanvasCLI: doctor and pages update
CanvasCLI-->>CanvasPublisher: Updated page response
CanvasPublisher-->>RuntimeAppApi: Canvas URL
RuntimeAppApi-->>Renderer: Publish receipt
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Around line 19-21: Update the README text describing the opt-in canvas-pp-cli
publishing workflow to state that it communicates directly with the configured
Canvas host, while clarifying that course data is not sent to an app-operated
cloud service. Preserve the existing per-page confirmation and
accessibility-gate requirements.
In `@src/app/e2e-api.ts`:
- Around line 435-446: The publish receipts in publishCanvasPage within
src/app/e2e-api.ts lines 435-446 and src/app/stub-api.ts lines 377-389 must use
the SHA-256 hexadecimal digest of the exact html instead of an HTML-length
marker. Update both implementations to compute and return the digest in
contentHash while preserving the remaining receipt fields and behavior.
In `@src/app/renderer/renderer.ts`:
- Around line 2107-2115: Replace currentPublishTarget’s reconstruction from
mutable Canvas state with an immutable publish-target snapshot captured when the
Canvas import/remediation succeeds. Clear that snapshot for non-Canvas runs, and
update the publish flow to use only the snapshot so later changes to
canvasBaseUrl, canvasCourseId, or selectedCanvasPageId cannot retarget the
repaired HTML.
- Around line 2096-2104: Update setPublishEnabled to serialize rapid
publishing-setting changes, preventing concurrent setCanvasPublishEnabled calls
from resolving out of order. Disable the related checkbox while the save is
pending or sequence requests so only the latest operation updates
state.publishStatus and state.error, while preserving the final render()
behavior.
In `@src/canvas/publish.ts`:
- Around line 197-204: Update available() to inspect the resolved result from
exec and return true only when its exitCode indicates success; continue
returning false for thrown errors and non-zero exits. Add a regression test
covering a resolved exec result with exitCode: 1 and verify the availability
result is false.
- Around line 56-94: Update defaultExec to listen for child.stdin errors and
settle the promise through the existing guarded cleanup path, preventing
unhandled errors when the CLI closes early. Update available() to consider the
probe’s exitCode and return false for non-zero exits, rather than treating every
resolved execution as available.
In `@src/runtime/app-api.ts`:
- Around line 806-818: Update the runtime around importCanvas and
publishCanvasPage to track each successfully imported (baseUrl, courseId,
pageId) tuple in current-session memory. Before calling publisher.publishPage,
require an exact tuple match and reject unimported pages; preserve the existing
publishing and accessibility checks. Add a direct API test verifying an
unimported page is rejected and publisher.publishPage is not invoked.
- Around line 818-831: Update the publish flow around publisher.publishPage and
the canvas_publishes insert to persist a pending audit intent before invoking
the CLI, then finalize that intent after Canvas succeeds. If finalization fails,
retain an explicit indeterminate audit state and do not report the operation as
a normal failed publish or encourage an unsafe retry.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8c7cf15c-5f74-43dd-9fe0-3884bbbb9869
📒 Files selected for processing (18)
README.mdsrc/app/bridge.test.tssrc/app/bridge.tssrc/app/channels.test.tssrc/app/channels.tssrc/app/e2e-api.tssrc/app/ipc.test.tssrc/app/ipc.tssrc/app/renderer/remediation.tssrc/app/renderer/renderer.tssrc/app/stub-api.tssrc/app/unavailable-api.tssrc/canvas/publish.test.tssrc/canvas/publish.tssrc/contracts/index.tssrc/runtime/app-api.test.tssrc/runtime/app-api.tssrc/storage/schema.ts
Comment on lines
+435
to
+446
| async publishCanvasPage(_baseUrl, courseId, pageId, html): Promise<CanvasPublishReceipt> { | ||
| failIfDown(); | ||
| if (!e2ePublishEnabled) { | ||
| throw new Error('Publishing to Canvas is disabled. Turn on "Allow publishing to Canvas" first.'); | ||
| } | ||
| return { | ||
| courseId, | ||
| pageId, | ||
| contentHash: `e2e-${html.length.toString(16)}`, | ||
| publishedAt: new Date().toISOString(), | ||
| canvasUrl: `https://e2e.instructure.test/courses/${courseId}/pages/${pageId}`, | ||
| }; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Make test-double publish receipts honor the SHA-256 contract.
Both implementations substitute HTML length for the required SHA-256 hexadecimal digest, allowing integration tests and stub workflows to accept invalid receipt semantics.
src/app/e2e-api.ts#L435-L446: compute the SHA-256 digest of the exacthtml.src/app/stub-api.ts#L377-L389: return the same contract-valid digest rather than a length marker.
📍 Affects 2 files
src/app/e2e-api.ts#L435-L446(this comment)src/app/stub-api.ts#L377-L389
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/e2e-api.ts` around lines 435 - 446, The publish receipts in
publishCanvasPage within src/app/e2e-api.ts lines 435-446 and
src/app/stub-api.ts lines 377-389 must use the SHA-256 hexadecimal digest of the
exact html instead of an HTML-length marker. Update both implementations to
compute and return the digest in contentHash while preserving the remaining
receipt fields and behavior.
Comment on lines
+2107
to
+2115
| /** The Canvas identity of the current remediation, when it was a live import. */ | ||
| function currentPublishTarget(): { baseUrl: string; courseId: string; pageId: string } | undefined { | ||
| if (state.sourceMode !== 'canvas') return undefined; | ||
| const baseUrl = state.canvasBaseUrl.trim(); | ||
| const courseId = state.canvasCourseId.trim(); | ||
| const pageId = state.selectedCanvasPageId; | ||
| if (!baseUrl || !courseId || !pageId) return undefined; | ||
| return { baseUrl, courseId, pageId }; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Bind the publish target to the completed Canvas import.
This reconstructs the target from mutable canvasBaseUrl, canvasCourseId, and selectedCanvasPageId state rather than the identity that produced remediateView. Changing the selection after remediation can therefore send one page’s repaired HTML to another imported page.
Capture an immutable target when import/remediation succeeds, clear it for non-Canvas runs, and publish only against that snapshot.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/renderer/renderer.ts` around lines 2107 - 2115, Replace
currentPublishTarget’s reconstruction from mutable Canvas state with an
immutable publish-target snapshot captured when the Canvas import/remediation
succeeds. Clear that snapshot for non-Canvas runs, and update the publish flow
to use only the snapshot so later changes to canvasBaseUrl, canvasCourseId, or
selectedCanvasPageId cannot retarget the repaired HTML.
Comment on lines
+806
to
+818
| async publishCanvasPage(baseUrl, courseId, pageId, html) { | ||
| if (!(await publishEnabledSetting())) { | ||
| throw new Error('Publishing to Canvas is disabled. Turn on "Allow publishing to Canvas" first.'); | ||
| } | ||
| // Server-side re-check of the accessibility gate on the EXACT HTML that | ||
| // would go out — a withheld badge refuses the publish regardless of what | ||
| // the renderer showed. The gated (allowlist-sanitized) html is what ships. | ||
| const gate = await enforceGate(html, gateDeps); | ||
| if (gate.badgeWithheld) { | ||
| const first = gate.conformance.blockers[0]?.message ?? 'accessibility blockers remain'; | ||
| throw new Error(`Refused to publish: the accessibility gate withheld the badge (${first}).`); | ||
| } | ||
| const { canvasUrl } = await publisher.publishPage({ baseUrl, courseId, pageId, html: gate.html }); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Enforce current-session import provenance in the runtime.
The method accepts arbitrary baseUrl, courseId, and pageId values without proving that this exact page was imported during the current session. Renderer checks can be bypassed through the API/IPC boundary, violating the publish guardrail.
Track imported page tuples in memory when importCanvas succeeds, then reject publishing unless the target tuple exists. Add a direct API test confirming an unimported page never reaches publisher.publishPage.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/runtime/app-api.ts` around lines 806 - 818, Update the runtime around
importCanvas and publishCanvasPage to track each successfully imported (baseUrl,
courseId, pageId) tuple in current-session memory. Before calling
publisher.publishPage, require an exact tuple match and reject unimported pages;
preserve the existing publishing and accessibility checks. Add a direct API test
verifying an unimported page is rejected and publisher.publishPage is not
invoked.
Comment on lines
+818
to
+831
| const { canvasUrl } = await publisher.publishPage({ baseUrl, courseId, pageId, html: gate.html }); | ||
| const receipt = { | ||
| courseId, | ||
| pageId, | ||
| contentHash: createHash('sha256').update(gate.html).digest('hex'), | ||
| publishedAt: new Date().toISOString(), | ||
| canvasUrl, | ||
| }; | ||
| const db = await database(); | ||
| await db.run( | ||
| `INSERT INTO canvas_publishes (course_id, page_id, content_hash, canvas_url, published_at) | ||
| VALUES (?, ?, ?, ?, ?)`, | ||
| [receipt.courseId, receipt.pageId, receipt.contentHash, receipt.canvasUrl, receipt.publishedAt], | ||
| ); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not report a failed publish after Canvas was already updated.
The external update succeeds before the audit insert. If SQLite then fails—such as from a full or locked disk—the method rejects even though Canvas changed and no receipt was recorded, encouraging an unsafe retry.
Persist a pending audit intent before invoking the CLI, finalize it after success, and retain an explicit indeterminate state if finalization fails.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/runtime/app-api.ts` around lines 818 - 831, Update the publish flow
around publisher.publishPage and the canvas_publishes insert to persist a
pending audit intent before invoking the CLI, then finalize that intent after
Canvas succeeds. If finalization fails, retain an explicit indeterminate audit
state and do not report the operation as a normal failed publish or encourage an
unsafe retry.
- available(): our spawn-based exec resolves on non-zero exit (unlike the catalog client's execFile), so a present-but-broken binary read as available. Gate on exitCode === 0. (+ regression test) - stdin: register an error listener before writing so an early child-close EPIPE can't become an unhandled exception; use end(data). - preflight: compare the FULL normalized base (scheme + host + path), not just the host, and build the receipt URL from the configured base — fixes http/https and path-hosted Canvas instances. (+ scheme-mismatch test) - renderer: never publish an empty body (would blank the live page) — guard htmlAfter before the write; drop the '' fallback. - renderer: disable the publish toggle while a save is in flight so rapid clicks can't persist out of order. - ipc: reject a non-boolean setCanvasPublishEnabled payload instead of coercing it (this toggle gates a write capability). - stub/e2e AppApi: return a real SHA-256 contentHash, matching the contract. - README: correct the blanket 'nothing leaves the device' claim — AI and doc processing are local, but Canvas import/publish talk to the connected Canvas. Skipped (verified not actionable): CodeRabbit's 'duplicate rows' critical is a false positive (single declaration, compiles, tests green); a publish/ setEnabled mutex is disproportionate for a single-user desktop app already guarded by publishBusy + the server-side gate re-check. Verify 602/0, Electron matrix 16/16, tsc clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Owner
Author
CodeRabbit review addressed (commit
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/app/stub-api.ts (1)
372-390: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winKeep stub publishing preconditions aligned with the runtime contract.
The runtime re-runs the accessibility gate on the exact HTML before publishing, but this stub only checks the toggle and hashes raw
html. Renderer/E2E tests using this implementation can therefore pass empty or ungated content that production rejects. Reuse shared validation where practical, or add explicit negative-case tests documenting the intentional difference.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/stub-api.ts` around lines 372 - 390, Update publishCanvasPage to enforce the same accessibility validation precondition as the runtime before hashing or returning the publish result, using the shared validation helper if available. Preserve the existing stubPublishEnabled check and validate the exact html passed to publishing so empty or ungated content is rejected consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Line 11: Update the README Requirements section and the publishing
instructions to consistently describe canvas-pp-cli as an optional prerequisite
required only for publishing, while preserving the statement that no additional
installation is needed for users who do not publish.
- Around line 7-12: Update the privacy statement near the language-model and
document-ingestion description to avoid claiming Canvas is the only network
destination; either scope the claim specifically to Canvas import/publishing
traffic or explicitly include user-provided URL ingestion as an exception,
consistent with the later SSRF documentation.
---
Nitpick comments:
In `@src/app/stub-api.ts`:
- Around line 372-390: Update publishCanvasPage to enforce the same
accessibility validation precondition as the runtime before hashing or returning
the publish result, using the shared validation helper if available. Preserve
the existing stubPublishEnabled check and validate the exact html passed to
publishing so empty or ungated content is rejected consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 06f08be3-162b-404a-ac34-90d59fd3104c
📒 Files selected for processing (8)
README.mdsrc/app/e2e-api.tssrc/app/ipc.tssrc/app/renderer/renderer.tssrc/app/stub-api.tssrc/canvas/publish.test.tssrc/canvas/publish.tssrc/runtime/app-api.test.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- src/app/ipc.ts
- src/app/e2e-api.ts
- src/runtime/app-api.test.ts
- src/canvas/publish.test.ts
- src/app/renderer/renderer.ts
- src/canvas/publish.ts
Comment on lines
+7
to
+12
| The language model and the document-ingestion pipeline both run on your machine | ||
| as bundled sidecars — no cloud AI service and no telemetry. The only network | ||
| calls are to the Canvas instance you choose to connect: reading pages to | ||
| remediate, and — strictly opt-in — publishing a repaired page back via the | ||
| separately installed `canvas-pp-cli`. Your Canvas token stays in the macOS | ||
| Keychain and is never sent anywhere else. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Avoid claiming Canvas is the only network destination.
The privacy section later documents URL ingestion and SSRF protection, implying that user-provided URLs may also be fetched. Narrow this claim to the Canvas import/publishing path or document the URL-ingestion exception.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` around lines 7 - 12, Update the privacy statement near the
language-model and document-ingestion description to avoid claiming Canvas is
the only network destination; either scope the claim specifically to Canvas
import/publishing traffic or explicitly include user-provided URL ingestion as
an exception, consistent with the later SSRF documentation.
| as bundled sidecars — no cloud AI service and no telemetry. The only network | ||
| calls are to the Canvas instance you choose to connect: reading pages to | ||
| remediate, and — strictly opt-in — publishing a repaired page back via the | ||
| separately installed `canvas-pp-cli`. Your Canvas token stays in the macOS |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Document the CLI prerequisite consistently.
This says publishing requires a separately installed canvas-pp-cli, while the Requirements section says end users install nothing else. List the CLI as an optional publishing prerequisite or qualify that statement.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` at line 11, Update the README Requirements section and the
publishing instructions to consistently describe canvas-pp-cli as an optional
prerequisite required only for publishing, while preserving the statement that
no additional installation is needed for users who do not publish.
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
1 participant
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.
Implements the approved product decision (2026-07-16): Canvas stays read-only by default; publishing a remediated page back is explicit opt-in, delegated to the separately installed
canvas-pp-cli.Guardrails (all must hold, else the path is invisible)
canvas-pp-clidetected on PATH (probe:agent-context) — the binary is not bundled, deliberatelydoctor→base_urlmust equal the import host) so a stale setting can never push to the wrong CanvasAudit trail
New
canvas_publishestable (schema v4): course, page, SHA-256 of the pushed HTML, timestamp. Receipt returned to the UI.Discipline
src/canvas/publish.tsmirrors the catalog client: arg-array spawn (no shell), HTML over stdin (never argv), validated ids, typed errors, timeoutssrc/canvas/http.tsremains GET-only by construction — untouched, tests still assert itVerification
npm run verify: 657 tests, 600 pass / 0 fail / 57 skip, tsc cleanpages update --stdin --agent --dry-runverified against the real CLI (request shape + auth + host)🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
canvas-pp-cliis installed.canvas-pp-cliusing a stored token.