diff --git a/.github/workflows/hybrid-workspace-boundary.yml b/.github/workflows/hybrid-workspace-boundary.yml new file mode 100644 index 000000000..63fc945d0 --- /dev/null +++ b/.github/workflows/hybrid-workspace-boundary.yml @@ -0,0 +1,24 @@ +name: Hybrid Workspace Boundary +on: + pull_request: + paths: + - 'apps/hybrid-workspace/**' + - '.github/workflows/hybrid-workspace-boundary.yml' +permissions: + contents: read +jobs: + browser: + name: Hybrid workspace browser boundary + runs-on: ubuntu-latest + timeout-minutes: 10 + env: + VERIFIED_HEAD: ${{ github.event.pull_request.head.sha }} + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 + - uses: actions/setup-node@v7 + with: + node-version: '24' + - run: npm ci --ignore-scripts + - run: node apps/hybrid-workspace/logic.test.mjs + - run: cd apps/web && npx --no-install playwright install --with-deps chromium + - run: node apps/hybrid-workspace/browser.test.mjs diff --git a/apps/hybrid-workspace/README.md b/apps/hybrid-workspace/README.md index aa0da7266..c5aa5538e 100644 --- a/apps/hybrid-workspace/README.md +++ b/apps/hybrid-workspace/README.md @@ -31,3 +31,12 @@ token. `python -m http.server` cannot resolve `libfx/browser` or inject The iframe uses `sandbox="allow-scripts"` only (no `allow-same-origin`) so student JS cannot read the parent page. + +## Deterministic boundary verification + +From the repository root run `node apps/hybrid-workspace/logic.test.mjs`. After installing locked dependencies and Playwright Chromium run `node apps/hybrid-workspace/browser.test.mjs`. + +The browser test serves actual app modules on loopback, substitutes fixture adapters for CDN editor/terminal/layout APIs, and blocks external requests. It covers Embed, edited Summarize, Extract, Ask escaping, Escalate, terminal input, parent-DOM sandbox confinement, and forbidden Gateway-key access. It does not certify live Monaco compatibility or Gateway execution. The dedicated PR workflow requires no production credentials. + +Helper tests passed locally on 2026-10-08; Chromium execution was blocked because no executable existed and its download returned an invalid archive. Browser status remains NOT RUN until the runner provides success. Parent #2202 stays open pending browser evidence and remaining coverage. `/studio` remains canonical. + diff --git a/apps/hybrid-workspace/browser.test.mjs b/apps/hybrid-workspace/browser.test.mjs new file mode 100644 index 000000000..311a42174 --- /dev/null +++ b/apps/hybrid-workspace/browser.test.mjs @@ -0,0 +1,61 @@ +// Browser plugin not available. Uses Playwright Chromium and actual app modules, +// with fixture adapters for CDN editor/terminal/layout APIs; no live Gateway. +import { chromium } from 'playwright'; +import { createServer } from 'node:http'; +import { readFile } from 'node:fs/promises'; +import assert from 'node:assert/strict'; +const root = new URL('./', import.meta.url); +const allowed = new Set(['index.html','app.js','logic.js','fx-adapter.js','styles.css']); +const server = createServer(async (req,res) => { + const name = req.url === '/' ? 'index.html' : req.url.slice(1); + if (!allowed.has(name)) { res.writeHead(404).end(); return; } + res.setHeader('Content-Type', name.endsWith('.js') ? 'text/javascript' : name.endsWith('.css') ? 'text/css' : 'text/html'); + res.end(await readFile(new URL(name,root))); +}); +await new Promise(r => server.listen(0,'127.0.0.1',r)); +let browser; +try { + browser = await chromium.launch({headless:true}); + const page = await browser.newPage(); + const origin = `http://127.0.0.1:${server.address().port}`; + const errors = []; + page.on('pageerror', e => errors.push(e.message)); + await page.route('**/*', route => route.request().url().startsWith(origin+'/') ? route.continue() : route.fulfill({status:200,body:'',contentType:'text/javascript'})); + await page.addInitScript(() => { + window.Split = () => {}; + const models = [], changes = []; + let active; + window.monaco = {editor:{createModel(value) {const m={getValue:()=>value,getValueInRange:()=>value,edit:v=>{value=v;}}; models.push(m);return m;},create(_el,opts){active=opts.model;return {setModel:m=>{active=m;},getModel:()=>active,getSelection:()=>({}),onDidChangeModelContent:fn=>changes.push(fn)};}}}; + window.require = (_deps,fn) => fn(); window.require.config = () => {}; + window.fixtureEdit = (index,value) => {active=models[index];active.edit(value);changes.forEach(fn=>fn());}; + window.Terminal = class {open(){} write(){} writeln(){} onData(fn){window.fixtureType=fn;}}; + Object.defineProperty(window,'AI_GATEWAY_API_KEY',{get(){throw new Error('Forbidden credential access');},set(){throw new Error('Forbidden credential write');}}); + }); + await page.goto(origin); + await page.waitForFunction(() => document.querySelectorAll('#files button').length===4); + assert.equal(await page.title(),'UVAI Hybrid Workspace'); + assert.equal(await page.locator('#preview').getAttribute('sandbox'),'allow-scripts'); + await page.locator('[data-action="embed"]').click(); + assert.match(await page.locator('#thread').innerText(),/Split screen stays locked/); + await page.evaluate(() => window.fixtureEdit(0,'Edited

New

')); + await page.locator('[data-action="summarize"]').click(); + assert.match(await page.locator('#thread').innerText(),/Edited/); + assert.equal(await page.locator('body').getAttribute('data-poison'),null); + const preview = page.frameLocator('#preview'); + await preview.locator('body[data-confined="1"]').waitFor(); + await page.evaluate(() => window.fixtureEdit(3,'')); + await page.locator('[data-action="summarize"]').click(); + assert.equal(await page.locator('#thread img').count(),0); + await page.locator('[data-action="extract"]').click(); + assert.equal(await page.locator('#thread img').count(),0); + await page.locator('#ask').fill(''); + await page.locator('#composer button').click(); + assert.equal(await page.locator('#thread img').count(),0); + await page.locator('[data-action="escalate"]').click(); + const ticketsBefore = await page.locator('#thread .ticket').count(); + await page.evaluate(() => window.fixtureType('ask file a ticket\r')); + assert.equal(await page.locator('#thread .ticket').count(), ticketsBefore + 1); + assert.match(await page.locator('#thread').innerText(),/session only/); + assert.deepEqual(errors,[]); + console.log(JSON.stringify({status:'VERIFIED',head:process.env.VERIFIED_HEAD || 'local-unpinned',surface:'hybrid-workspace',boundary:'Chromium DOM + sandbox',externalServices:'disabled',editor:'fixture adapter',checks:['identity','embed','edited summary','extract','ask escaping','terminal escalation','sandbox confinement','credential access trap','console health']})); +} finally { await browser?.close(); await new Promise(r=>server.close(r)); } diff --git a/apps/hybrid-workspace/logic.js b/apps/hybrid-workspace/logic.js index 7f74f35fb..06208d81e 100644 --- a/apps/hybrid-workspace/logic.js +++ b/apps/hybrid-workspace/logic.js @@ -5,34 +5,46 @@ export function escapeHtml(s) { } export function injectVirtualAssets(html, assets) { - let out = String(html); + // Native HTML parsing gives real attribute, duplicate, comment and end-tag + // semantics. This substitutes virtual assets; it is not an HTML sanitizer. + // Edited JavaScript executes only in the opaque-origin sandboxed preview. + const original = String(html); + const doc = new DOMParser().parseFromString(original, 'text/html'); + let changed = false; for (const [name, body] of Object.entries(assets)) { - if (name.endsWith(".css")) { - out = out.replace( - new RegExp(`]*href=["']${name}["'][^>]*>`, "i"), - ``, - ); - } else if (name.endsWith(".js")) { - out = out.replace( - new RegExp(`]*src=["']${name}["'][^>]*>\\s*`, "i"), - ``, - ); + if (name.endsWith('.css')) { + for (const link of doc.querySelectorAll('link[href]')) { + if (link.getAttribute('href') !== name) continue; + const style = doc.createElement('style'); + if (link.hasAttribute('media')) style.setAttribute('media', link.getAttribute('media')); + style.textContent = String(body).replace(/<\/style/gi, '<\\/style'); + link.replaceWith(style); + changed = true; + } + } else if (name.endsWith('.js')) { + for (const script of doc.querySelectorAll('script[src]')) { + if (script.getAttribute('src') !== name) continue; + script.removeAttribute('src'); + script.textContent = String(body).replace(/<\/script/gi, '<\\/script'); + changed = true; + } } } - return out; + if (!changed) return original; + return (doc.doctype ? '\n' : '') + doc.documentElement.outerHTML; } export function shouldEscalate(text, action) { if (action === "escalate") return true; const q = String(text || "").toLowerCase(); - return /\\b(escalate|file a ticket|human support|support ticket)\\b/.test(q); + return /\b(escalate|file a ticket|human support|support ticket)\b/.test(q); } export function summarizeFrom(files) { const lesson = files["lesson.md"]?.value || ""; const html = files["index.html"]?.value || ""; - const title = (html.match(/([^<]+)<\\/title>/i) || [, "Untitled"])[1]; - const headings = [...html.matchAll(/<h1[^>]*>([^<]+)<\\/h1>/gi)].map((m) => m[1]); + const title = (html.match(/<title>([^<]+)<\/title>/i) || [, "Untitled"])[1]; + const headings = [...html.matchAll(/<h1[^>]*>([^<]+)<\/h1>/gi)].map((m) => m[1]); const excerpt = lesson.split("\n").slice(0, 8).join("\n"); return { title, @@ -50,3 +62,4 @@ export function parseTermCommand(line) { if (cmd === "ask") return { action: "ask", text }; return { action: "ask", text: raw }; } + diff --git a/apps/hybrid-workspace/logic.test.mjs b/apps/hybrid-workspace/logic.test.mjs index f35a0611f..a2a97879f 100644 --- a/apps/hybrid-workspace/logic.test.mjs +++ b/apps/hybrid-workspace/logic.test.mjs @@ -1,3 +1,5 @@ +import { JSDOM } from 'jsdom'; +globalThis.DOMParser = new JSDOM('').window.DOMParser; import { escapeHtml, injectVirtualAssets, shouldEscalate, summarizeFrom, parseTermCommand } from "./logic.js"; import assert from "node:assert/strict"; @@ -7,6 +9,7 @@ const html = `<html><head><link rel="stylesheet" href="styles.css" /></head><bod const injected = injectVirtualAssets(html, { "styles.css": "body{color:red}", "app.js": "1" }); assert.match(injected, /<style>body\{color:red\}<\/style>/); assert.match(injected, /<script>1<\/script>/); +assert.match(injectVirtualAssets('<script src="app.js"></script >', {"app.js":"1"}), /<script>1<\/script>/); assert.doesNotMatch(injected, /src="app\.js"/); assert.equal(shouldEscalate("help me understand this CSS", "ask"), false); @@ -25,3 +28,15 @@ assert.deepEqual(parseTermCommand("summarize"), { action: "summarize", text: "" assert.deepEqual(parseTermCommand("ask why css"), { action: "ask", text: "why css" }); console.log("hybrid-workspace logic tests passed"); +assert.equal(shouldEscalate("please file a ticket", "ask"), true); +const reordered = `<link href='styles.css' rel='stylesheet'><script defer src='app.js'></script>`; +const hostile = injectVirtualAssets(reordered, { "styles.css": 'a{content:"$&"}', "app.js": 'console.log("</script><img>")' }); +assert.match(hostile, /content:"\$&"/); +assert.doesNotMatch(hostile, /<\/script><img>/); +assert.equal(injectVirtualAssets('<script src="appXjs"></script>', { "app.js": "1" }), '<script src="appXjs"></script>'); +assert.equal(injectVirtualAssets('<link href="missing.css">', { "styles.css": "1" }), '<link href="missing.css">'); +for (const markup of ['<link data-href="styles.css">', `<link title=' href="styles.css"'>`, '<script data-src="app.js"></script>', `<script title=' src="app.js"'></script>`]) { + assert.equal(injectVirtualAssets(markup, {"styles.css":"bad", "app.js":"bad"}), markup); +} +assert.equal(summarizeFrom({"index.html":{value:'<title>Edited

New

'},"lesson.md":{value:''}}).title,'Edited'); + diff --git a/apps/web/src/lib/__tests__/chat-pack-binding-boundary.test.ts b/apps/web/src/lib/__tests__/chat-pack-binding-boundary.test.ts new file mode 100644 index 000000000..2312d7ced --- /dev/null +++ b/apps/web/src/lib/__tests__/chat-pack-binding-boundary.test.ts @@ -0,0 +1,19 @@ +import { expect, it, vi } from 'vitest'; +vi.mock('@/lib/video-pack', () => ({ buildIdentityPack: vi.fn(() => { throw new Error('Identity builder must not run for malformed input'); }), isIdentityOnlyPack: vi.fn() })); +vi.mock('@/lib/video-pack-store', () => ({ getPackRecordWithMeta: vi.fn() })); +import { buildIdentityPack } from '@/lib/video-pack'; +import { parseChatPackBinding, resolveChatPackGrounding } from '@/lib/chat-pack-grounding'; +it('rejects malformed video and pack identifiers before constructing identity', () => { + for (const video_id of ['bad', 'auJzb1D-fag-extra', 'https://youtu.be/auJzb1D-fag']) { + expect(parseChatPackBinding({video_id})).toBeNull(); + expect(parseChatPackBinding({pack_id:`vp:v0:${video_id}`})).toBeNull(); + } +}); +it('rejects directly supplied malformed bindings with400 and no identity lookup', async () => { + expect(await resolveChatPackGrounding({videoId:'bad',packId:'vp:v0:bad'})).toMatchObject({ok:false,status:400,code:'pack_id_mismatch'}); + expect(buildIdentityPack).not.toHaveBeenCalled(); +}); +it('accepts only matching canonical identifiers', () => { + expect(parseChatPackBinding({pack_id:'vp:v0:auJzb1D-fag'})).toEqual({videoId:'auJzb1D-fag',packId:'vp:v0:auJzb1D-fag'}); + expect(parseChatPackBinding({video_id:'auJzb1D-fag',pack_id:'vp:v0:jNQXAC9IVRw'})).toBeNull(); +}); diff --git a/apps/web/src/lib/chat-pack-grounding.ts b/apps/web/src/lib/chat-pack-grounding.ts index f564a687b..fe5c8789e 100644 --- a/apps/web/src/lib/chat-pack-grounding.ts +++ b/apps/web/src/lib/chat-pack-grounding.ts @@ -3,9 +3,10 @@ import { isIdentityOnlyPack, type VideoPackV0Json, } from '@/lib/video-pack'; +import { resolveYouTubeSourceId } from '@/lib/video-source-identity'; import { getPackRecordWithMeta, type VideoPackRecord } from '@/lib/video-pack-store'; -const PACK_ID_RE = /^vp:v0:[A-Za-z0-9_-]+$/; +const PACK_ID_RE = /^vp:v0:[A-Za-z0-9_-]{11}$/; const MAX_TRANSCRIPT_CHARS = 12_000; const MAX_VISUAL_EVENTS = 24; const MAX_SOP_STEPS = 32; @@ -43,7 +44,7 @@ export function parseChatPackBinding(input: { if (!videoId && rawPack && PACK_ID_RE.test(rawPack)) { videoId = rawPack.slice('vp:v0:'.length); } - if (!videoId) return null; + if (!videoId || resolveYouTubeSourceId(videoId) !== videoId) return null; const expectedPackId = `vp:v0:${videoId}`; if (rawPack) { @@ -150,6 +151,9 @@ function readyPackFromRecord( export async function resolveChatPackGrounding( binding: ChatPackBinding, ): Promise { + if (resolveYouTubeSourceId(binding.videoId) !== binding.videoId || binding.packId !== `vp:v0:${binding.videoId}`) { + return packFailure(400, 'pack_id_mismatch', 'A canonical video and pack identity is required.'); + } const identity = buildIdentityPack(binding.videoId); const lookup = await getPackRecordWithMeta(identity.provenance.source_hash); @@ -170,3 +174,4 @@ export async function resolveChatPackGrounding( return readyPackFromRecord(lookup.record, binding); } + diff --git a/docs/EXCEPTION_HANDLING_STANDARDS.md b/docs/EXCEPTION_HANDLING_STANDARDS.md index 66315ffb2..69afa5e18 100644 --- a/docs/EXCEPTION_HANDLING_STANDARDS.md +++ b/docs/EXCEPTION_HANDLING_STANDARDS.md @@ -6,4 +6,7 @@ Critical paths (auth, G.A.T.E., pack persistence, deployment handoff): 2. Never swallow auth or payment failures—return explicit HTTP status / reason codes. 3. Bare `except:` is forbidden in new code (Ruff BLE001 in critical modules over time). -Remediation is incremental; prioritize `backend/api/v1/router.py`, `services/agents/`, and MCP orchestrator entrypoints. +Remediation is incremental; issue #2168 remains open. The previously prioritized `backend/api/v1/router.py` path is absent from the repository tree inspected on 2026-10-08. Current audit targets include `apps/backend/main.py`, `src/agents/mcp_tools/deployment_tool.py`, agent orchestrators under `src/agents/`, and agent services under `src/youtube_extension/services/agents/`. This document is policy, not evidence that every critical handler has been remediated. + +Before changing a handler, capture its concrete failure mode and caller contract. Use specific exceptions for expected failures; outer-boundary exceptions must log context and propagate an explicit failure. Do not convert security/payment failures into successful defaults. A repository-wide count is insufficient acceptance evidence; attach route/agent failure tests for each repaired critical path. + diff --git a/docs/MIGRATION_TRUTH.md b/docs/MIGRATION_TRUTH.md index a7bf4122e..9c37739d2 100644 --- a/docs/MIGRATION_TRUTH.md +++ b/docs/MIGRATION_TRUTH.md @@ -1,19 +1,15 @@ -# SQL model vs migration truth (AUDIT-007 / #2169) +# SQL model and migration truth (AUDIT-007 / #2169) -## Current state +## Current repository evidence — 2026-10-08 -| Area | Location | Notes | -| --- | --- | --- | -| Model mixins | `src/youtube_extension/backend/models/base.py` | Timestamps, tenant, UUID, soft-delete | -| Model modules | `src/youtube_extension/backend/models/*.py` | Multiple domain modules | -| Alembic revisions | `src/youtube_extension/backend/migrations/versions/` | **3** committed revisions (`001`–`003`) | +The previous inventory in this file named backend model modules and three Alembic revisions. That inventory is obsolete: the current recursive repository tree does not contain `src/youtube_extension/backend/models/`, `src/youtube_extension/backend/migrations/`, or `tests/unit/test_migration_truth.py`. `alembic.ini` still points to the missing migration directory. Do not treat the old count guard or directory list as verified current coverage. -## Policy +One SQL migration is present at `apps/web/supabase/migrations/20260912214530_create_private_studio_chat_ownership.sql`. Its presence alone does not prove that all live tables, tenant policies, deployed revisions, or Python persistence models are reconciled. -1. Any model used in production request paths must have a matching migration revision or an explicit “non-prod / optional store” doc entry here. -2. New tables/columns require Alembic revision in the same PR as model changes. -3. CI guard: `tests/unit/test_migration_truth.py` fails if revision count drops or model package disappears. +## Acceptance and blocking requirements -## Follow-up (tracked) +Issue #2169 remains open. Before creating migrations, identify the actual active database and owning service, inventory current persistence models and deployed revision/RLS state using approved read-only access, and map each model to its authoritative migration history. The missing legacy directory needs an explicit retirement or restoration decision; do not silently manufacture a replacement migration chain. -Generate incremental migrations for unmigrated model domains after inventory sign-off. Do not auto-generate destructive migrations in CI. +No live schema comparison, database access, migration generation, or migration application was performed in this correction. A destructive migration requires separate approval. New tables/columns must accompany the matching migration and fixture validation in the same PR. + +Historical claim retained for audit: the previous file asserted three committed revisions (`001`–`003`) and a `test_migration_truth.py` guard. That claim is not valid for the inspected current tree. diff --git a/src/agents/mcp_tools/deployment_tool.py b/src/agents/mcp_tools/deployment_tool.py index b78676a2d..523ef3573 100644 --- a/src/agents/mcp_tools/deployment_tool.py +++ b/src/agents/mcp_tools/deployment_tool.py @@ -205,20 +205,28 @@ async def _initialize_git(self, project_dir: Path) -> dict[str, Any]: # Initial commit # SECURITY: subprocess call uses hardcoded command list. - subprocess.run( + add_result = subprocess.run( ["git", "add", "."], cwd=str(project_dir), capture_output=True, timeout=30 ) - subprocess.run( + if add_result.returncode != 0: + logger.error("Deployment handoff failed", extra={"stage": "git_add", "returncode": add_result.returncode}) + return {"success": False, "error": "Git staging failed"} + + commit_result = subprocess.run( ["git", "commit", "-m", "Initial commit - AI generated project"], cwd=str(project_dir), capture_output=True, timeout=30 ) + if commit_result.returncode != 0: + logger.error("Deployment handoff failed", extra={"stage": "git_commit", "returncode": commit_result.returncode}) + return {"success": False, "error": "Git initial commit failed"} + logger.info("✅ Git initialized") return {"success": True} @@ -341,7 +349,8 @@ async def _deploy_to_vercel( break if not deployment_url: - deployment_url = f"https://{project_name}.vercel.app" + logger.error("Deployment output lacked verified URL", extra={"stage": "vercel_output"}) + return {"success": False, "error": "Vercel returned no verified deployment URL"} return { "success": True, @@ -363,11 +372,13 @@ def _get_github_username(self) -> str: timeout=10, env={**os.environ, "GH_TOKEN": self.github_token} ) - if result.returncode == 0: + if result.returncode == 0 and result.stdout.strip(): return result.stdout.strip() - except: - pass - return "unknown" + except (subprocess.TimeoutExpired, OSError) as exc: + logger.error("GitHub identity lookup failed", extra={"stage": "github_identity", "error_class": type(exc).__name__}) + raise RuntimeError("GitHub identity lookup failed") from exc + logger.error("GitHub identity lookup failed", extra={"stage": "github_identity", "returncode": result.returncode}) + raise RuntimeError("GitHub identity lookup failed") # Singleton instance @@ -384,3 +395,4 @@ def get_deployment_tool() -> DeploymentMCPTool: MCP_TOOLS = { "deploy_to_github_and_vercel": get_deployment_tool().deploy_to_github_and_vercel } + diff --git a/tests/unit/test_deployment_handoff_failures.py b/tests/unit/test_deployment_handoff_failures.py new file mode 100644 index 000000000..8eb8119c1 --- /dev/null +++ b/tests/unit/test_deployment_handoff_failures.py @@ -0,0 +1,40 @@ +"""No live deployment: prove failures cannot become successful handoffs.""" +import asyncio +import importlib.util +import subprocess +from pathlib import Path +from types import SimpleNamespace +from unittest.mock import patch + +import pytest + +spec = importlib.util.spec_from_file_location('deployment_handoff', Path(__file__).parents[2] / 'src/agents/mcp_tools/deployment_tool.py') +module = importlib.util.module_from_spec(spec) +spec.loader.exec_module(module) + +def tool(): + instance = object.__new__(module.DeploymentMCPTool) + instance.github_token = None + instance.vercel_token = None + return instance + +@pytest.mark.parametrize('failure_stage', [1, 2]) +def test_git_failure_stops_handoff(tmp_path, failure_stage): + results = [SimpleNamespace(returncode=0, stderr='') for _ in range(3)] + results[failure_stage] = SimpleNamespace(returncode=1, stderr='private provider output') + with patch.object(module.subprocess, 'run', side_effect=results) as run: + result = asyncio.run(tool()._initialize_git(tmp_path)) + assert result['success'] is False + assert run.call_count == failure_stage + 1 + assert 'private provider output' not in str(result) + +@pytest.mark.parametrize('failure', [subprocess.TimeoutExpired('gh', 10), FileNotFoundError('gh')]) +def test_identity_failure_never_fabricates_unknown_owner(failure): + with patch.object(module.subprocess, 'run', side_effect=failure), pytest.raises(RuntimeError, match='identity lookup failed'): + tool()._get_github_username() + +def test_missing_deployment_url_is_not_success(tmp_path): + with patch.object(module.subprocess, 'run', return_value=SimpleNamespace(returncode=0, stdout='completed', stderr='')): + result = asyncio.run(tool()._deploy_to_vercel(tmp_path, 'example', None)) + assert result['success'] is False + assert 'deployment_url' not in result