Feat/tck_testing

[gabewillen]

Description

WIP: Complete Create2 TCK Test Suite Implementation

This PR implements all remaining tests for the Create2 TCK test suite (Creating Relationships), achieving 100% test coverage (24/24 tests passing). This is a work-in-progress that includes the complete
implementation plus cleanup of development artifacts.

Related Issues

Closes #

Type of Change

• 🐛 Bug fix (non-breaking change that fixes an issue)
• ✨ New feature (non-breaking change that adds functionality)
• 💥 Breaking change (fix or feature that would cause existing functionality to not work as expected)
• 📝 Documentation update
• 🔧 Build/CI configuration change
• ♻️ Code refactoring (no functional changes)
• ⚡ Performance improvement
• ✅ Test improvement

Changes Made

• ✅ Implemented all 3 remaining Create2 error validation tests (Tests 19, 20, 23)
• ✅ Added comprehensive relationship pattern validation in semantic analyzer
• ✅ Fixed Makefile to remove references to deleted test files
• ✅ Fixed library path in tck_test_clauses_create.c (build/libgraph → ../build/libgraph)
• ✅ Removed .agents/ and tck/ directories from git tracking (kept locally for development)
• ✅ Updated .gitignore to exclude tck/ directory

Testing

• All existing tests pass (make test)
• Added new tests for new functionality
• Tested with Valgrind for memory leaks
• Tested with AddressSanitizer
• Manual testing performed
• Performance testing performed (if applicable)

Test Evidence

Create2 Test Suite Results:

• Total: 24/24 tests passing (100%)
• Test 19: Undirected relationship validation ✅
• Test 20: Bidirectional relationship validation ✅
• Test 23: Relationship variable rebinding validation ✅

Overall CREATE Test Suite:

• 78 tests total
• 56 passing (71.8%)
• 22 failing (expected - other test suites)
• 2 ignored

No regressions introduced - all previously passing tests still pass.

Documentation

• Updated API documentation
• Updated README.md (if needed)
• Updated CHANGELOG.md
• Added code comments where necessary
• Updated examples (if applicable)

Checklist

• My code follows the project's code style guidelines
• I have performed a self-review of my code
• I have commented my code, particularly in hard-to-understand areas
• My changes generate no new warnings
• I have checked for memory leaks (WIP)
• I have updated the documentation accordingly (WIP)
• My changes are compatible with the supported platforms (Linux x86_64)

Breaking Changes

N/A

Performance Impact

• No performance impact
• Performance improved
• Performance slightly degraded (justified because...)
• Performance significantly degraded (needs discussion)

Screenshots / Examples

Error Validation Examples:

-- Test 19: Undirected relationships now properly rejected
CREATE (a)-[:FOO]-(b)
-- Error: SyntaxError: RequiresDirectedRelationship

-- Test 20: Bidirectional relationships now properly rejected  
CREATE (a)<-[:FOO]->(b)
-- Error: SyntaxError: RequiresDirectedRelationship

-- Test 23: Relationship variable rebinding now properly rejected
MATCH ()-[r]->() CREATE ()-[r]->()
-- Error: SyntaxError: VariableAlreadyBound

Additional Context

Implementation Details:

Added comprehensive relationship pattern validation to validateCreatePattern() in src/cypher/cypher-planner.c (lines 314-345):
1. Direction validation: Checks iFlags to detect undirected/bidirectional patterns
2. Variable rebinding validation: Checks if relationship variable already exists in context

Files Modified:
- src/cypher/cypher-planner.c - Added relationship validation
- tests/Makefile - Removed deleted test references
- tests/tck_test_clauses_create.c - Fixed library path
- .gitignore - Added tck/ exclusion

Reviewer Notes

⚠️ This is a WIP PR - Still needs:
- Memory leak testing with Valgrind
- Documentation updates
- CHANGELOG.md update

Ready for Review:
- Core functionality complete
- All tests passing
- No regressions
- Code follows style guidelines

---
For Maintainers:

- Code review completed
- All CI checks passing
- Documentation reviewed
- Version number updated (if needed)
- CHANGELOG.md updated

<!-- CURSOR_SUMMARY -->
---

> [!NOTE]
> Implement full MERGE (nodes/relationships) with ON CREATE/ON MATCH, add validation and execution, update planner/physical/storage, and introduce organized TCK test harness.
> 
> - **Language/Parsing**:
>   - Add `ON` keyword to lexer; extend parser to support `MERGE` with `ON CREATE/ON MATCH SET`, path binding, multi-labels, pattern lists, and multiple `MATCH/CREATE` clauses; forbid `WHERE` after `MERGE` and variable-length rels in `MERGE`.
> - **Validation (Planner)**:
>   - Create-pattern validation (variable (re)binding, directed-only rels, type consistency); capture MERGE metadata (nodes/rels, props, set ops).
> - **Physical Plan**:
>   - Carry MERGE details; serialize match/set ops to JSON; proper exec state cleanup.
> - **Storage**:
>   - Normalize column names (`node_id`→`id`, `from_node/to_node`→`source/target`); update CRUD helpers accordingly.
> - **Write/Execution**:
>   - Implement CREATE/DELETE/SET and MERGE (nodes/relationships) including `ON CREATE`/`ON MATCH`, sanitization, property accumulation, rollback, and helpers (stmt cache, JSON rendering).
> - **Tests/Tooling**:
>   - Add/organize TCK runner and generated tests (MATCH/CREATE/DELETE/WITH); restructure `tests/Makefile`; introduce Python harness and placeholder suites; minor path fixes and .gitignore updates.
> 
> <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 2512fc413f268c4d032282982dc8d54859145553. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup>
<!-- /CURSOR_SUMMARY -->

[Copilot]

Pull Request Overview

This PR implements comprehensive TCK (Technology Compatibility Kit) test coverage for the Create2 test suite and makes several foundational improvements to the codebase. The changes achieve 100% test coverage for Create2 (24/24 tests passing) while also refactoring test infrastructure and fixing database schema inconsistencies.

Key Changes:

• Complete Create2 TCK test suite implementation with relationship pattern validation
• Database schema standardization (node_id→id, edge_id→id, from_node→source, to_node→target)
• Enhanced MERGE clause parsing with ON CREATE/ON MATCH support
• Test infrastructure refactoring with shared test utilities and placeholder suites
• Python test cleanup (removal of development scripts)

Reviewed Changes

Copilot reviewed 67 out of 81 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
tests/test_*.c	New test infrastructure with common utilities and placeholder suites
tests/Makefile	Refactored build system with output directory organization
tests/tck_test_clauses_*.c	Updated TCK tests with proper library paths and implementations
src/cypher/cypher-storage.c	Fixed column name consistency (node_id→id, from_node→source, etc.)
src/cypher/cypher-parser.c	Enhanced MERGE parsing with ON clauses and multi-label support
src/cypher/cypher-lexer.c	Added ON token for MERGE clause support
src/cypher/cypher-logical-plan.c	Improved memory management for MERGE details
src/cypher/cypher-executor.c	Enhanced error message propagation
tests/python_support/*	New Python test harness infrastructure
tests/*.py (deleted)	Removed development/debug Python scripts

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

'except' clause does nothing but pass and there is no explanatory comment.

[cursor]

Bug: Shallow Copies: Memory Corruption Risk

The cypherFunctionMin and cypherFunctionMax functions perform shallow copies of argument values by direct assignment (*pResult = apArgs[0]), which creates aliased pointers when copying CYPHER_VALUE_STRING, CYPHER_VALUE_LIST, or CYPHER_VALUE_MAP types. When cypherEvaluateFunction later calls cypherValueDestroy(&aValues[i]) to clean up arguments, these shared pointers get freed, leaving pResult pointing to deallocated memory. This causes use-after-free bugs when the result value is accessed or destroyed.

 

[cursor]

Bug: Inconsistent Resource Handling Leaks

Memory leak in cypherValueSetString when the value was previously a list or map. The function only frees the existing string if pValue->type is already CYPHER_VALUE_STRING, but if the value was previously a CYPHER_VALUE_LIST or CYPHER_VALUE_MAP, those resources won't be freed. Other setter functions like cypherValueSetInteger, cypherValueSetFloat, and cypherValueSetBoolean correctly call cypherValueDestroy(pValue) first to free any existing resources before assigning the new value.

src/cypher/cypher-execution-context.c#L260-L278

sqlite-graph/src/cypher/cypher-execution-context.c

Lines 260 to 278 in 3d29394

	*/
	int cypherValueSetString(CypherValue *pValue, const char *zString) {
	if( !pValue ) return SQLITE_MISUSE;
	/* Free existing string if any */
	if( pValue->type == CYPHER_VALUE_STRING && pValue->u.zString ) {
	sqlite3_free(pValue->u.zString);
	}
	pValue->type = CYPHER_VALUE_STRING;
	if( zString ) {
	pValue->u.zString = sqlite3_mprintf("%s", zString);
	if( !pValue->u.zString ) return SQLITE_NOMEM;
	} else {
	pValue->u.zString = NULL;
	}
	return SQLITE_OK;

 

---

[cursor]

Bug: Workflow Input Undefined, Validation Misleading

The workflow references inputs.skip_validation in the branch validation step, but this input is not defined in the workflow_dispatch inputs section. This causes the branch validation to always run (since the undefined input evaluates to false), making the error message on line 74 misleading. Either the skip_validation input needs to be added to the workflow inputs, or the conditional and error message should be removed.