diff --git a/CLAUDE.md b/CLAUDE.md index c5cb917ed..fc2c78191 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -14,9 +14,15 @@ This file provides guidance to Claude Code (claude.ai/code) when working with co - Pre-commit Check: `make pre-commit` or `npm run check` ## Important Workflow Requirements -- ALWAYS run `npm run lint` before any git operations (`git add`, `git commit`, etc.) -- The linter will automatically fix formatting issues, so this must be run before committing -- Failing to run the linter may result in style issues and commit failures +- ⚠️ NEVER BYPASS PRE-COMMIT HOOKS! NEVER use `git commit -n` or `--no-verify` ⚠️ +- ALWAYS run `make pre-commit` or at minimum `npm run ts-check` and `npm run lint` before committing +- The full process must always be: + 1. Make changes + 2. Run `npm run ts-check` to verify TypeScript types + 3. Run `npm run lint` to fix style issues (will auto-fix many problems) + 4. Run `npm run test` to verify functionality (or at least `npm run test-min`) + 5. ONLY THEN commit changes with plain `git commit` (NO FLAGS!) +- Bypassing these checks will lead to broken builds, failed tests, and PRs that cannot be merged - ALWAYS use HEREDOC syntax for complex shell commands, especially those containing quotes, newlines, or special characters: ```bash gh pr create --title "Title" --body "$(cat <<'EOF' diff --git a/app.ts b/app.ts index 6ad06fd79..48c62eabd 100755 --- a/app.ts +++ b/app.ts @@ -52,7 +52,7 @@ import systemdSocket from 'systemd-socket'; import _ from 'underscore'; import urljoin from 'url-join'; -import {unwrap} from './lib/assert.js'; +import {setBaseDirectory, unwrap} from './lib/assert.js'; import * as aws from './lib/aws.js'; import * as normalizer from './lib/clientstate-normalizer.js'; import {GoldenLayoutRootStruct} from './lib/clientstate-normalizer.js'; @@ -89,8 +89,7 @@ import {ElementType} from './shared/common-utils.js'; import {CompilerInfo} from './types/compiler.interfaces.js'; import type {Language, LanguageKey} from './types/languages.interfaces.js'; -// Used by assert.ts -global.ce_base_directory = new URL('.', import.meta.url); +setBaseDirectory(new URL('.', import.meta.url)); (nopt as any).invalidHandler = (key: string, val: unknown, types: unknown[]) => { logger.error( diff --git a/lib/assert.ts b/lib/assert.ts index 897983eec..627ba2eeb 100644 --- a/lib/assert.ts +++ b/lib/assert.ts @@ -28,32 +28,47 @@ import path from 'node:path'; import {isString} from '../shared/common-utils.js'; import {parse} from '../shared/stacktrace.js'; +// Helper for cross-platform path handling in tests +export function normalizePath(filePath: string): string { + return filePath.split(path.sep).join('/'); +} + +let ce_base_directory = ''; + const filePrefix = 'file://'; -function removeFileProtocol(path: string) { +export function setBaseDirectory(base_url_path: URL) { + ce_base_directory = base_url_path.pathname; +} + +// Explicitly export for testing purposes - not part of the public API +export function removeFileProtocol(path: string) { if (path.startsWith(filePrefix)) { return path.slice(filePrefix.length); } return path; } -function check_path(parent: URL, directory: string) { +// Explicitly export for testing purposes - not part of the public API +export function check_path(parent: string, directory: string) { // https://stackoverflow.com/a/45242825/15675011 - const relative = path.relative(parent.pathname, directory); + const relative = path.relative(parent, directory); if (relative && !relative.startsWith('..') && !path.isAbsolute(relative)) { - return relative; + // Normalize separators to forward slashes for consistent behavior across platforms + return normalizePath(relative); } return false; } -function get_diagnostic() { +// Explicitly export for testing purposes - not part of the public API +export function get_diagnostic() { const e = new Error(); const trace = parse(e); if (trace.length >= 4) { const invoker_frame = trace[3]; if (invoker_frame.fileName && invoker_frame.lineNumber) { // Just out of an abundance of caution... - const relative = check_path(global.ce_base_directory, removeFileProtocol(invoker_frame.fileName)); + const relative = check_path(ce_base_directory, removeFileProtocol(invoker_frame.fileName)); if (relative) { try { const file = fs.readFileSync(invoker_frame.fileName, 'utf8'); diff --git a/lib/global.ts b/lib/global.ts index e2f69712a..22d767088 100644 --- a/lib/global.ts +++ b/lib/global.ts @@ -27,7 +27,6 @@ import {HandlerConfig} from './handlers/route-api.js'; declare global { // var is required - var ce_base_directory: URL; var handler_config: HandlerConfig; /* eslint-enable no-var */ } diff --git a/test/assert-tests.ts b/test/assert-tests.ts new file mode 100644 index 000000000..ab17b690a --- /dev/null +++ b/test/assert-tests.ts @@ -0,0 +1,218 @@ +// Copyright (c) 2025, Compiler Explorer Authors +// All rights reserved. +// +// Redistribution and use in source and binary forms, with or without +// modification, are permitted provided that the following conditions are met: +// +// * Redistributions of source code must retain the above copyright notice, +// this list of conditions and the following disclaimer. +// * Redistributions in binary form must reproduce the above copyright +// notice, this list of conditions and the following disclaimer in the +// documentation and/or other materials provided with the distribution. +// +// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" +// AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE +// IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE +// ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE +// LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR +// CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF +// SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS +// INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN +// CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) +// ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE +// POSSIBILITY OF SUCH DAMAGE. + +import path from 'node:path'; +import {beforeEach, describe, expect, it} from 'vitest'; + +import {assert, check_path, removeFileProtocol, setBaseDirectory, unwrap, unwrapString} from '../lib/assert.js'; + +describe('Assert module', () => { + // Reset the base directory for each test + beforeEach(() => { + setBaseDirectory(new URL('file:///test/path/')); + }); + + describe('setBaseDirectory', () => { + it('should set the base directory from a URL', () => { + const testUrl = new URL('file:///test/path/'); + setBaseDirectory(testUrl); + + // We can verify the base directory was set by testing check_path + // which uses the base directory internally + const result = check_path('/test/path/', '/test/path/subdir/file.js'); + expect(result).toEqual('subdir/file.js'); + }); + }); + + describe('removeFileProtocol', () => { + it('should remove file:// prefix', () => { + expect(removeFileProtocol('file:///path/to/file')).toEqual('/path/to/file'); + }); + + it('should not modify paths without file:// prefix', () => { + expect(removeFileProtocol('/path/to/file')).toEqual('/path/to/file'); + }); + }); + + describe('check_path', () => { + it('should return relative path for valid subdirectories', () => { + // This test now uses normalized paths with forward slashes for cross-platform compatibility + expect(check_path('/root', '/root/sub/file.js')).toEqual('sub/file.js'); + }); + + it('should return false for paths outside the parent directory', () => { + expect(check_path('/root/sub', '/root/file.js')).toBe(false); + expect(check_path('/root', '/other/file.js')).toBe(false); + }); + + it('should return false for absolute paths', () => { + expect(check_path('/root', '/root')).toBe(false); + }); + }); + + describe('get_diagnostic function components', () => { + // Since get_diagnostic relies on stack traces which are hard to test directly, + // we test each component separately to verify the logic is correct + + describe('stack trace handling', () => { + it('should correctly process file paths with removeFileProtocol', () => { + const pathWithProtocol = 'file:///path/to/file.js'; + expect(removeFileProtocol(pathWithProtocol)).toEqual('/path/to/file.js'); + + const pathWithoutProtocol = '/path/to/file.js'; + expect(removeFileProtocol(pathWithoutProtocol)).toEqual('/path/to/file.js'); + }); + + it('should properly determine if a path is within the base directory using check_path', () => { + // Valid paths inside base directory - these paths use forward slashes regardless of platform + // for cross-platform test compatibility + const testRoot = '/base/dir'; + const testFile = path.join(testRoot, 'file.js'); + const testSubdirFile = path.join(testRoot, 'subdir', 'file.js'); + + expect(check_path(testRoot, testFile)).toEqual('file.js'); + expect(check_path(testRoot, testSubdirFile)).toEqual('subdir/file.js'); + + // Invalid paths + expect(check_path('/base/dir', '/other/path/file.js')).toBe(false); + expect(check_path('/base/dir', '/base')).toBe(false); + }); + + it('should handle the combination of file protocol removal and path checking', () => { + const baseDir = '/base/dir'; + const filePathWithProtocol = 'file:///base/dir/file.js'; + + // First remove protocol (as done in get_diagnostic) + const filePath = removeFileProtocol(filePathWithProtocol); + + // Then check if path is within base directory (as done in get_diagnostic) + const relativePath = check_path(baseDir, filePath); + + // The relative path should be 'file.js' because it's directly in the base dir + expect(relativePath).toEqual('file.js'); + }); + }); + + describe('file content extraction', () => { + it('should correctly extract a specific line from file content', () => { + // This mimics the line extraction logic in get_diagnostic + const fileContent = 'line1\nline2\nline3\nline4\nline5'; + const lines = fileContent.split('\n'); + + expect(lines[0].trim()).toEqual('line1'); + expect(lines[2].trim()).toEqual('line3'); + expect(lines[4].trim()).toEqual('line5'); + }); + }); + }); + + describe('assert', () => { + it('should not throw for truthy values', () => { + expect(() => assert(true)).not.toThrow(); + expect(() => assert(1)).not.toThrow(); + expect(() => assert('string')).not.toThrow(); + expect(() => assert({})).not.toThrow(); + }); + + it('should throw for falsy values', () => { + expect(() => assert(false)).toThrow('Assertion failed'); + expect(() => assert(0)).toThrow('Assertion failed'); + expect(() => assert('')).toThrow('Assertion failed'); + expect(() => assert(null)).toThrow('Assertion failed'); + expect(() => assert(undefined)).toThrow('Assertion failed'); + }); + + it('should include custom message in error', () => { + expect(() => assert(false, 'Custom message')).toThrow(/Custom message/); + }); + + it('should include extra info in error message', () => { + const extra = {a: 1, b: 2}; + expect(() => assert(false, 'Message', extra)).toThrow(JSON.stringify([extra])); + }); + + it('should include the assertion message', () => { + try { + assert(false, 'Test assertion'); + // Should not reach here + expect(true).toBe(false); + } catch (e: any) { + expect(e.message).toContain('Assertion failed: Test assertion'); + } + }); + }); + + describe('unwrap', () => { + it('should return the value for non-null/undefined values', () => { + expect(unwrap(5)).toEqual(5); + expect(unwrap('test')).toEqual('test'); + expect(unwrap(false)).toEqual(false); + expect(unwrap(0)).toEqual(0); + + const obj = {test: true}; + expect(unwrap(obj)).toBe(obj); + }); + + it('should throw for null values', () => { + expect(() => unwrap(null)).toThrow('Unwrap failed'); + }); + + it('should throw for undefined values', () => { + expect(() => unwrap(undefined)).toThrow('Unwrap failed'); + }); + + it('should include custom message in error', () => { + expect(() => unwrap(null, 'Custom unwrap message')).toThrow(/Custom unwrap message/); + }); + + it('should include extra info in error message', () => { + const extra = {reason: 'test'}; + expect(() => unwrap(null, 'Message', extra)).toThrow(JSON.stringify([extra])); + }); + }); + + describe('unwrapString', () => { + it('should return string values', () => { + expect(unwrapString('test')).toEqual('test'); + expect(unwrapString('')).toEqual(''); + }); + + it('should throw for non-string values', () => { + expect(() => unwrapString(123)).toThrow('String unwrap failed'); + expect(() => unwrapString(null)).toThrow('String unwrap failed'); + expect(() => unwrapString(undefined)).toThrow('String unwrap failed'); + expect(() => unwrapString({})).toThrow('String unwrap failed'); + expect(() => unwrapString([])).toThrow('String unwrap failed'); + }); + + it('should include custom message in error', () => { + expect(() => unwrapString(123, 'Expected string value')).toThrow(/Expected string value/); + }); + + it('should include extra info in error message', () => { + const extra = {value: 123, type: 'number'}; + expect(() => unwrapString(123, 'Message', extra)).toThrow(JSON.stringify([extra])); + }); + }); +});