brooktrails/directory-pacifier
2
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

feat: Add typed approvalstore client for the approval-store schema #3

Merged
rcsheets merged 3 commits from feat/approvalstore-client into main 2026-04-13 04:41:59 +00:00
Conversation 3 Commits 3 Files changed 3 +1027 -1
rcsheets commented 2026-04-13 04:28:39 +00:00
Owner
Copy link

Provides one INSERT-per-action surface over schema/001-003: ProposePlan,
ExpandOperation, Decide, RecordExecution, GrantTrust, plus view-backed
ListPendingPlans / ListApprovedReadyToExecute / LookupEffectiveTrust.
Content hashes are computed by the client (via internal/contenthash) so
callers cannot diverge from the canonical form.

Integration tests exercise each method under the appropriate Postgres
role via SET LOCAL ROLE inside a transaction, confirming that role grants
are enforced end-to-end through the typed surface. Client-side input
validation (decision/expires_at, empty JSON, expected_target_count) is
covered without touching a DB.

Extend the schema-test CI job to run the new integration tests alongside
the existing dbtest suite.

Co-Authored-By: Claude Opus 4.6 (1M context) noreply@anthropic.com

Provides one INSERT-per-action surface over schema/001-003: ProposePlan, ExpandOperation, Decide, RecordExecution, GrantTrust, plus view-backed ListPendingPlans / ListApprovedReadyToExecute / LookupEffectiveTrust. Content hashes are computed by the client (via internal/contenthash) so callers cannot diverge from the canonical form. Integration tests exercise each method under the appropriate Postgres role via SET LOCAL ROLE inside a transaction, confirming that role grants are enforced end-to-end through the typed surface. Client-side input validation (decision/expires_at, empty JSON, expected_target_count) is covered without touching a DB. Extend the schema-test CI job to run the new integration tests alongside the existing dbtest suite. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add typed approvalstore client for the approval-store schema
Some checks failed
test / go-test (pull_request) Successful in 14s
Details
test / schema-test (pull_request) Failing after 26s
Details
97734c50d0 
Provides one INSERT-per-action surface over schema/001-003: ProposePlan,
ExpandOperation, Decide, RecordExecution, GrantTrust, plus view-backed
ListPendingPlans / ListApprovedReadyToExecute / LookupEffectiveTrust.
Content hashes are computed by the client (via internal/contenthash) so
callers cannot diverge from the canonical form.

Integration tests exercise each method under the appropriate Postgres
role via SET LOCAL ROLE inside a transaction, confirming that role grants
are enforced end-to-end through the typed surface. Client-side input
validation (decision/expires_at, empty JSON, expected_target_count) is
covered without touching a DB.

Extend the schema-test CI job to run the new integration tests alongside
the existing dbtest suite.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
pr-reviewer-bot commented 2026-04-13 04:28:41 +00:00
Collaborator
Copy link

Automated review by pr-reviewer v0.15.0 | Safety Check | anthropic | tracking id r-dc70f9-9ea3e1
This is an AI-generated review and may contain mistakes.

Status: ✅ Completed

✅ Verdict: LGTM — Nothing alarming found - this is a well-structured database client with comprehensive tests and proper role-based access controls.
Complexity: moderate

This PR adds a typed Go client for an approval-store database schema with proper role-based access control validation.

Key observations:

  • Well-structured database client with comprehensive input validation
  • Integration tests exercise role permissions end-to-end via SET LOCAL ROLE
  • Proper handling of PostgreSQL-specific types (intervals, UUIDs, JSONB)
  • Content hash computation ensures data integrity
  • CI integration looks correct (adds test path to existing workflow)

Security considerations reviewed:

  • ✅ Role-based access control properly enforced through PostgreSQL roles
  • ✅ Input validation prevents empty JSON and invalid parameters
  • ✅ SQL injection protection via parameterized queries
  • ✅ No hardcoded credentials or secrets

Data integrity:

  • ✅ Content hashing prevents tampering/divergence
  • ✅ Append-only design with proper constraints
  • ✅ Transaction handling in tests ensures isolation

The code quality is high with proper error handling, comprehensive tests, and clear separation of concerns. The integration test approach using role switching is particularly well done.

*Automated review by [pr-reviewer](https://git.brooktrails.org/brooktrails/pr-reviewer) v0.15.0 | Safety Check | anthropic | tracking id `r-dc70f9-9ea3e1`* *This is an AI-generated review and may contain mistakes.* **Status:** ✅ Completed --- **✅ Verdict: LGTM** — Nothing alarming found - this is a well-structured database client with comprehensive tests and proper role-based access controls. **Complexity:** moderate This PR adds a typed Go client for an approval-store database schema with proper role-based access control validation. **Key observations:** - Well-structured database client with comprehensive input validation - Integration tests exercise role permissions end-to-end via `SET LOCAL ROLE` - Proper handling of PostgreSQL-specific types (intervals, UUIDs, JSONB) - Content hash computation ensures data integrity - CI integration looks correct (adds test path to existing workflow) **Security considerations reviewed:** - ✅ Role-based access control properly enforced through PostgreSQL roles - ✅ Input validation prevents empty JSON and invalid parameters - ✅ SQL injection protection via parameterized queries - ✅ No hardcoded credentials or secrets **Data integrity:** - ✅ Content hashing prevents tampering/divergence - ✅ Append-only design with proper constraints - ✅ Transaction handling in tests ensures isolation The code quality is high with proper error handling, comprehensive tests, and clear separation of concerns. The integration test approach using role switching is particularly well done.
Compare REAL-backed sample_rate with tolerance in trust round-trip test
Some checks failed
test / go-test (pull_request) Successful in 14s
Details
test / schema-test (pull_request) Failing after 26s
Details
8e8432a66b 
trust_grants.sample_rate is a REAL column (float32), so a 0.1 literal
round-tripped through it loses precision and no longer equals the
original float64 0.1 exactly. Compare with a 1e-6 tolerance instead.
Also dereference the pointer in the error message so future failures
report the value, not an address.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
pr-reviewer-bot commented 2026-04-13 04:30:40 +00:00
Collaborator
Copy link

Automated review by pr-reviewer v0.15.0 | Safety Check | anthropic | tracking id r-dc7170-81d022
This is an AI-generated review and may contain mistakes.

Status: ✅ Completed

✅ Verdict: LGTM — This is a well-structured database client implementation with comprehensive integration tests and proper role-based security enforcement.
Complexity: moderate

Safety Check Review

This PR adds a typed Go client for the approval-store schema with comprehensive integration tests. The code is well-structured and follows good practices:

Positive Observations

  • Proper input validation (JSON validation, required fields, business rules)
  • Role-based security is tested end-to-end with SET LOCAL ROLE
  • Content hashing prevents divergence from canonical forms
  • Integration tests cover full lifecycle scenarios
  • Error handling is consistent and informative
  • Uses standard PostgreSQL driver (pgx)

No Alarming Issues Found

  • No hardcoded credentials or secrets
  • No obvious security vulnerabilities
  • No breaking changes to existing APIs
  • Input validation prevents common injection vectors
  • Database operations use parameterized queries
  • Test cleanup prevents data leakage between tests

The 1000+ lines of new code are legitimate implementation and comprehensive tests, not vendored dependencies. The CI integration properly extends existing schema tests.

*Automated review by [pr-reviewer](https://git.brooktrails.org/brooktrails/pr-reviewer) v0.15.0 | Safety Check | anthropic | tracking id `r-dc7170-81d022`* *This is an AI-generated review and may contain mistakes.* **Status:** ✅ Completed --- **✅ Verdict: LGTM** — This is a well-structured database client implementation with comprehensive integration tests and proper role-based security enforcement. **Complexity:** moderate # Safety Check Review This PR adds a typed Go client for the approval-store schema with comprehensive integration tests. The code is well-structured and follows good practices: ## Positive Observations - Proper input validation (JSON validation, required fields, business rules) - Role-based security is tested end-to-end with `SET LOCAL ROLE` - Content hashing prevents divergence from canonical forms - Integration tests cover full lifecycle scenarios - Error handling is consistent and informative - Uses standard PostgreSQL driver (pgx) ## No Alarming Issues Found - No hardcoded credentials or secrets - No obvious security vulnerabilities - No breaking changes to existing APIs - Input validation prevents common injection vectors - Database operations use parameterized queries - Test cleanup prevents data leakage between tests The 1000+ lines of new code are legitimate implementation and comprehensive tests, not vendored dependencies. The CI integration properly extends existing schema tests.
pr-reviewer-bot commented 2026-04-13 04:31:08 +00:00
Collaborator
Copy link

Automated review by pr-reviewer v0.15.0 | Full Review | anthropic | tracking id r-dc718b-2a1cb8
This is an AI-generated review and may contain mistakes.

Status: ✅ Completed

✅ Verdict: Approve — This is a well-architected, comprehensive typed client library with excellent test coverage and security considerations.
Complexity: moderate

Code Review: Typed approvalstore client

This PR introduces a comprehensive typed Go client for the approval-store schema. The implementation is well-designed with strong security practices and thorough test coverage.

Strengths

Architecture & Design

  • Excellent abstraction: The DBTX interface allows flexible usage with pools, connections, or transactions
  • Security-first design: Role-based access control is enforced at the database level, with the client simply issuing SQL and letting Postgres reject unauthorized operations
  • Content hash integrity: Client-computed hashes prevent divergence from canonical forms and enable independent verification by the RPC server
  • Immutable append-only model: Aligns perfectly with audit requirements and data integrity

Input Validation & Error Handling

  • Client-side validation: Appropriate pre-flight checks (empty JSON, required fields, decision validation) prevent unnecessary database round-trips
  • Comprehensive error wrapping: All database errors are properly wrapped with context
  • Edge case handling: Proper handling of nullable fields, empty JSON, and type conversions

Test Coverage

  • Full lifecycle testing: TestFullLifecycle exercises the complete propose→expand→decide→execute flow using actual Postgres roles
  • Security validation: TestRoleEnforcement confirms database-level access controls work through the typed interface
  • Edge cases covered: Validation failures, content hash reproducibility, Postgres interval conversions, and no-row scenarios
  • Integration approach: Tests use real Postgres with proper schema, providing confidence in production behavior

Code Quality

  • Clear documentation: Package and function docs explain the role-based model and usage patterns
  • Consistent patterns: All INSERT methods follow the same structure (validate→hash→insert→return UUID)
  • Type safety: Proper use of enums for Decision and TrustLevel values
  • Resource management: Proper cleanup of database resources with deferred rows.Close()

Technical Implementation Notes

PostgreSQL Integration

  • Interval handling: The durationToInterval/intervalToDuration conversion properly handles the impedance mismatch between Go and Postgres time representations
  • Float precision: Test acknowledges and properly handles float32/float64 precision differences
  • Parameter binding: Correct use of parameterized queries prevents SQL injection

JSON Handling

  • Validation strategy: The validateJSON function prevents empty JSON while allowing flexible content
  • NULL semantics: rawOrNil helper properly handles optional JSON fields
  • Type safety: Consistent use of json.RawMessage for deferred parsing

Minor Observations

  1. Line 300: The comment about "lossy but harmless approximation" for interval conversion is well-documented and appropriate for the use case

  2. Lines 79, 142: Client-side validation provides good user experience by catching common errors early

  3. Test structure: The role-switching pattern with SET LOCAL ROLE in transactions is elegant and provides strong end-to-end validation

CI Integration

The workflow changes properly extend the schema-test job to include the new integration tests, maintaining the existing database setup while expanding coverage.

Verdict

This is high-quality code that follows Go best practices and implements a secure, well-tested database client. The role-based security model is properly implemented, the test coverage is comprehensive, and the code is maintainable and well-documented. Ready to merge.

*Automated review by [pr-reviewer](https://git.brooktrails.org/brooktrails/pr-reviewer) v0.15.0 | Full Review | anthropic | tracking id `r-dc718b-2a1cb8`* *This is an AI-generated review and may contain mistakes.* **Status:** ✅ Completed --- **✅ Verdict: Approve** — This is a well-architected, comprehensive typed client library with excellent test coverage and security considerations. **Complexity:** moderate # Code Review: Typed approvalstore client This PR introduces a comprehensive typed Go client for the approval-store schema. The implementation is well-designed with strong security practices and thorough test coverage. ## Strengths ### Architecture & Design - **Excellent abstraction**: The `DBTX` interface allows flexible usage with pools, connections, or transactions - **Security-first design**: Role-based access control is enforced at the database level, with the client simply issuing SQL and letting Postgres reject unauthorized operations - **Content hash integrity**: Client-computed hashes prevent divergence from canonical forms and enable independent verification by the RPC server - **Immutable append-only model**: Aligns perfectly with audit requirements and data integrity ### Input Validation & Error Handling - **Client-side validation**: Appropriate pre-flight checks (empty JSON, required fields, decision validation) prevent unnecessary database round-trips - **Comprehensive error wrapping**: All database errors are properly wrapped with context - **Edge case handling**: Proper handling of nullable fields, empty JSON, and type conversions ### Test Coverage - **Full lifecycle testing**: `TestFullLifecycle` exercises the complete propose→expand→decide→execute flow using actual Postgres roles - **Security validation**: `TestRoleEnforcement` confirms database-level access controls work through the typed interface - **Edge cases covered**: Validation failures, content hash reproducibility, Postgres interval conversions, and no-row scenarios - **Integration approach**: Tests use real Postgres with proper schema, providing confidence in production behavior ### Code Quality - **Clear documentation**: Package and function docs explain the role-based model and usage patterns - **Consistent patterns**: All INSERT methods follow the same structure (validate→hash→insert→return UUID) - **Type safety**: Proper use of enums for `Decision` and `TrustLevel` values - **Resource management**: Proper cleanup of database resources with deferred `rows.Close()` ## Technical Implementation Notes ### PostgreSQL Integration - **Interval handling**: The `durationToInterval`/`intervalToDuration` conversion properly handles the impedance mismatch between Go and Postgres time representations - **Float precision**: Test acknowledges and properly handles float32/float64 precision differences - **Parameter binding**: Correct use of parameterized queries prevents SQL injection ### JSON Handling - **Validation strategy**: The `validateJSON` function prevents empty JSON while allowing flexible content - **NULL semantics**: `rawOrNil` helper properly handles optional JSON fields - **Type safety**: Consistent use of `json.RawMessage` for deferred parsing ## Minor Observations 1. **Line 300**: The comment about "lossy but harmless approximation" for interval conversion is well-documented and appropriate for the use case 2. **Lines 79, 142**: Client-side validation provides good user experience by catching common errors early 3. **Test structure**: The role-switching pattern with `SET LOCAL ROLE` in transactions is elegant and provides strong end-to-end validation ## CI Integration The workflow changes properly extend the schema-test job to include the new integration tests, maintaining the existing database setup while expanding coverage. ## Verdict This is high-quality code that follows Go best practices and implements a secure, well-tested database client. The role-based security model is properly implemented, the test coverage is comprehensive, and the code is maintainable and well-documented. Ready to merge.
Run schema integration test packages sequentially in CI
All checks were successful
test / go-test (pull_request) Successful in 14s
Details
test / schema-test (pull_request) Successful in 52s
Details
2cc8c3c1cc 
dbtest and approvalstore both hit the same Postgres instance and each
starts its tests by DELETE-ing all rows. go test runs packages in
parallel by default, so concurrent cleanups raced with assertions —
one package could wipe rows the other had just inserted and was about
to query, producing spurious empty-result failures.

Force sequential package execution with -p 1. Tests within a package
are already sequential (no t.Parallel() calls).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
rcsheets deleted branch feat/approvalstore-client 2026-04-13 04:42:01 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
pr-reviewer-bot
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
brooktrails/directory-pacifier!3
No description provided.