test(e2e): add chat acceptance bundle#412
Merged
Merged
Conversation
📝 WalkthroughWalkthroughAdds a Node.js Chat Acceptance Gate
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 2
🤖 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 `@scripts/verify/chat-acceptance.mjs`:
- Around line 198-200: The Windows cmd.exe invocation built in the
chat-acceptance runner is not safely escaped, so special characters can be
re-parsed by cmd.exe. Update the command construction in the logic around
spawnCommand/spawnArgs and the helper that feeds quoteArg to either use proper
cmd-safe escaping for all shell metacharacters or avoid assembling a single /c
string altogether. Make sure the fix covers every place that builds the Windows
command line so paths containing &, ^, %, (, or ) are handled correctly.
In `@tests/contract/scripts/verify-chat-acceptance.mjs`:
- Around line 44-74: The current test only covers the all-gates-skipped path, so
it never exercises the redaction logic in verify-chat-acceptance.mjs. Update the
test around spawnSync and the manifest assertions to run at least one real gate
with a controlled fake corepack on PATH that emits secret-like text, then verify
the output produced by shortenText() / redactSecretLike() contains redacted
markers and does not include raw Authorization or client_secret values.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f3cba9b6-0cc1-4b25-9c68-bd8e4ee84fb1
📒 Files selected for processing (7)
AGENTS.mdapp/desktop/package.jsonapp/package.jsonapp/web/package.jsondocs/progress/MASTER.mdscripts/verify/chat-acceptance.mjstests/contract/scripts/verify-chat-acceptance.mjs
Comment on lines
+198
to
+200
| const spawnCommand = process.platform === 'win32' ? process.env.ComSpec ?? 'cmd.exe' : command; | ||
| const spawnArgs = process.platform === 'win32' | ||
| ? ['/d', '/s', '/c', [command, ...commandArgs].map(quoteArg).join(' ')] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Escape the Windows cmd.exe /c string (scripts/verify/chat-acceptance.mjs:198-200, 262-264). quoteArg() only covers spaces and " here, so a path containing &, ^, %, (, or ) can be re-parsed by cmd.exe and break the runner on Windows. Use cmd-safe escaping or avoid building a single /c string.
🤖 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 `@scripts/verify/chat-acceptance.mjs` around lines 198 - 200, The Windows
cmd.exe invocation built in the chat-acceptance runner is not safely escaped, so
special characters can be re-parsed by cmd.exe. Update the command construction
in the logic around spawnCommand/spawnArgs and the helper that feeds quoteArg to
either use proper cmd-safe escaping for all shell metacharacters or avoid
assembling a single /c string altogether. Make sure the fix covers every place
that builds the Windows command line so paths containing &, ^, %, (, or ) are
handled correctly.
Comment on lines
+44
to
+74
| const outputPath = path.join(tmpRoot, 'chat-acceptance-manifest.json'); | ||
| const run = spawnSync(process.execPath, [ | ||
| scriptPath, | ||
| '--repo-root', repoRoot, | ||
| '--artifact-root', tmpRoot, | ||
| '--output-path', outputPath, | ||
| '--skip-shared-unit', | ||
| '--skip-desktop-playwright', | ||
| '--skip-web-playwright', | ||
| '--skip-desktop-visual-qa', | ||
| '--skip-web-visual-qa', | ||
| ], { cwd: repoRoot, encoding: 'utf8' }); | ||
| assert(run.status === 0, 'runner can write manifest with all rows skipped', `${run.stdout}\n${run.stderr}`); | ||
| assert(fs.existsSync(outputPath), 'runner writes output manifest'); | ||
|
|
||
| if (fs.existsSync(outputPath)) { | ||
| const jsonText = fs.readFileSync(outputPath, 'utf8'); | ||
| const json = JSON.parse(jsonText); | ||
| assert(json.schema === 'agenthub.chat_acceptance_bundle.v1', 'manifest schema is explicit'); | ||
| assert(json.status === 'skipped', 'manifest records skipped status when no gates ran'); | ||
| assert(json.real_tested === false, 'manifest records real_tested=false'); | ||
| assert(json.boundaries.real_tokendance_id_login === false, 'manifest records no real login'); | ||
| assert(json.boundaries.real_cli_or_model_api === false, 'manifest records no real CLI/model/API'); | ||
| assert(json.boundaries.packaged_desktop === false, 'manifest records no packaged Desktop'); | ||
| assert(json.rows.filter((row) => row.name === 'shared-chat-unit' && row.evidence_level === 'fixture-unit').length === 1, 'manifest has shared unit row'); | ||
| assert(json.rows.filter((row) => row.name === 'desktop-chat-playwright' && row.evidence_level === 'playwright-ui').length === 1, 'manifest has Desktop Playwright row'); | ||
| assert(json.rows.filter((row) => row.name === 'web-chat-playwright' && row.evidence_level === 'playwright-ui').length === 1, 'manifest has Web Playwright row'); | ||
| assert(json.rows.filter((row) => row.name === 'desktop-chat-visual-qa' && row.evidence_level === 'visual-qa').length === 1, 'manifest has Desktop Visual QA row'); | ||
| assert(json.rows.filter((row) => row.name === 'web-chat-visual-qa' && row.evidence_level === 'visual-qa').length === 1, 'manifest has Web Visual QA row'); | ||
| assert(!/sk-[A-Za-z0-9]|client_secret|Authorization:\s*Bearer/.test(jsonText), 'manifest is redacted'); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
This never verifies the redaction path.
Lines 50-54 skip every gate, so the emitted manifest only contains skip reasons; the shortenText() / redactSecretLike() branch in scripts/verify/chat-acceptance.mjs never runs. A redaction regression would still pass this test. Please run at least one gate through a controlled fake corepack on PATH that prints Authorization: Bearer ... or client_secret=..., then assert the manifest contains the redacted markers instead of the raw secret.
🤖 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 `@tests/contract/scripts/verify-chat-acceptance.mjs` around lines 44 - 74, The
current test only covers the all-gates-skipped path, so it never exercises the
redaction logic in verify-chat-acceptance.mjs. Update the test around spawnSync
and the manifest assertions to run at least one real gate with a controlled fake
corepack on PATH that emits secret-like text, then verify the output produced by
shortenText() / redactSecretLike() contains redacted markers and does not
include raw Authorization or client_secret values.
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.
Summary
apppackage script and focused Desktop/Web package scripts.Task Issue
Closes #389
S.U.P.E.R Review
Tests
node --check scripts/verify/chat-acceptance.mjsnode tests/contract/scripts/verify-chat-acceptance.mjs --repo-root .corepack.cmd pnpm --dir app install --frozen-lockfile --prefer-offlinenode scripts/verify/chat-acceptance.mjs --repo-root . --command-timeout-sec 420- passed 5/5corepack.cmd pnpm --dir app run test:acceptance:chat-flow -- --command-timeout-sec 420- passed 5/5pwsh ./scripts/verify/verify-doc-ssot.ps1pwsh ./scripts/verify/verify-project-skills.ps1pwsh ./scripts/verify/verify-real-e2e-contract.ps1git diff --checkEvidence Boundary
real_tested=false.fixture-unit,playwright-ui,visual-qa.Manifest Evidence
Latest local manifest status:
passed; rows passed: 5/5; generated byapppackage script at.tmp/chat-acceptance/run-46648/chat-acceptance-manifest.json.Project Governance
AGENTS.mdupdated to avoid growing large PowerShell scripts for cross-platform frontend acceptance gates.docs/progress/MASTER.mdupdated for T4.1 implementation state.Summary by CodeRabbit
New Features
Tests