Skip to content

fix(controller): sanitize subtask text and persist plans atomically - #438

Open
N1neSun wants to merge 1 commit into
vxcontrol:mainfrom
N1neSun:fix/subtask-plan-persistence
Open

N1neSun wants to merge 1 commit into
vxcontrol:mainfrom
N1neSun:fix/subtask-plan-persistence

Conversation

@N1neSun

@N1neSun N1neSun commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

Description of the Change

Problem

Model-generated subtask titles or descriptions can contain NUL bytes (\x00), which PostgreSQL does not accept in text fields. Saving such a subtask fails with:

pq: invalid byte sequence for encoding "UTF8": 0x00

Subtask generation and plan replacement write rows individually without a transaction. During refinement, existing planned subtasks are deleted before the replacement is fully saved. If a later insert fails, the task can be left waiting with an incomplete plan and missing report steps.

Solution

  • Add a shared database.SaveSubtaskPlan operation for subtask generation, refinement, and assistant plan patches.
  • Apply the existing SanitizeUTF8 helper to titles and descriptions before insertion. It removes NUL bytes, replaces malformed UTF-8, and preserves valid text.
  • Perform deletion of replaced subtasks and insertion of the complete new plan in one transaction.
  • Roll back the transaction if any delete or insert fails.
  • Invalidate cached subtask workers only after a successful commit.
  • Add regression tests for text normalization, partial-write rollback, delete and commit failures, and cache invalidation.

Related issue: None.

Type of Change

  • 🐛 Bug fix (non-breaking change which fixes an issue)
  • 🚀 New feature (non-breaking change which adds functionality)
  • 💥 Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • 📚 Documentation update
  • 🔧 Configuration change
  • 🧪 Test update
  • 🛡️ Security update

Areas Affected

  • Core Services (Frontend UI/Backend API)
  • AI Agents (Researcher/Developer/Executor)
  • Security Tools Integration
  • Memory System (Vector Store/Knowledge Base)
  • Monitoring Stack (Grafana/OpenTelemetry)
  • Analytics Platform (Langfuse)
  • External Integrations (LLM/Search APIs)
  • Documentation
  • Infrastructure/DevOps

Testing and Verification

Test Configuration

PentAGI Version: "Commit 6fdda78, based on b611fbe"
Docker Version: "Not used for local verification"
Host OS: "Windows"
LLM Provider: "Not used; automated tests use mocks"
Enabled Features: []

Test Steps

Run from backend/:

  1. Run database and controller tests:

    go test ./pkg/database ./pkg/controller
  2. Run provider tests:

    go test ./pkg/providers

    To run the remaining tests excluding the two known failing groups:

    go test -skip 'TestCatalog_ReadmeProviderSectionsMatchTheShippedConfig|TestPerformer_PerformAgentChain_WritesAMissingScopeToGraphitiAsADash' ./pkg/providers
  3. Run static checks and build the affected packages:

    go vet ./pkg/database ./pkg/controller ./pkg/providers
    go build ./pkg/database ./pkg/controller ./pkg/providers
  4. Verify formatting and whitespace with gofmt and git diff --check.

Test Results

  • Database and controller tests passed.
  • Provider tests passed when excluding the two known failing groups listed above.
  • Both failing groups were reproduced using the pre-change provider implementation:
    • README provider-heading checks for Gemini and Qwen.
    • Graphiti episode-content comparisons affected by Windows CRLF line endings.
  • go vet, package builds, and git diff --check passed.
  • Regression tests verify that NUL-containing text is normalized and a failed second insert rolls back the entire transaction.
  • No live PostgreSQL, Docker deployment, or live LLM smoke test was performed.

Security Considerations

This change does not alter authentication, authorization, permissions, or credential handling. Text normalization prevents unsupported characters from reaching PostgreSQL; it does not replace parameterized queries.

Adds github.com/DATA-DOG/go-sqlmock v1.5.2 for database regression tests only. It is not used by production code.

Performance Impact

Plan persistence adds one transaction per generation or replacement operation. Text normalization is linear in the length of each title and description.

LLM calls remain outside the transaction. No performance benchmark was performed.

Documentation Updates

  • README.md updates
  • API documentation updates
  • Configuration documentation updates
  • GraphQL schema updates
  • Other: Updated backend/docs/database.md to describe text normalization, atomic plan persistence, and cache invalidation after commit.

Deployment Notes

Rebuild and deploy the backend to apply the fix.

No database migrations, new environment variables, API changes, or configuration changes are required.

This change prevents future partial plan writes. It does not automatically restore subtasks lost during earlier failed executions; affected tasks may require explicit replanning.

Checklist

Code Quality

  • My code follows the project's coding standards
  • I have added/updated necessary documentation
  • I have added tests to cover my changes
  • All new and existing tests pass
  • I have run go fmt and go vet (for Go code)
  • I have run pnpm run lint (for TypeScript/JavaScript code)

Security

  • I have considered security implications
  • Changes maintain or improve the security model
  • Sensitive information has been properly handled

Compatibility

  • Changes are backward compatible
  • Breaking changes are clearly marked and documented
  • Dependencies are properly updated

Documentation

  • Documentation is clear and complete
  • Comments are added for non-obvious code
  • API changes are documented

Additional Notes

No frontend, REST API, GraphQL schema, or generated sqlc files were changed.

Custom implementations of database.Querier used for plan persistence must also provide the transactional SaveSubtaskPlan operation. Unsupported implementations fail explicitly rather than falling back to non-transactional writes.

The unchecked test-suite item reflects the two baseline failures documented above. JavaScript linting, breaking-change documentation, and API-change documentation are not applicable to this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant