design: execution boundary ownership (X-5 alignment)#10
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the X-5 Manual UAT Runbook to reflect significant changes to the API contract and testing approach. The PR description claims to establish the "Execution Layer as single source of truth" for request_id, trace_id, and timestamp, aligning with X-5 UAT requirements.
Changes:
- Updated API endpoint from
/executeto/invokewith new request/response structure - Changed field naming from
trace_idtorequest_idand addedexecuted_attimestamp field - Reorganized UAT test scenarios with more detailed determinism and session isolation tests
- Simplified grep-based architectural guardrail checks and observability validation
- Removed references to foundational design documents and constitutional compliance checks
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # 9. Architectural Guardrails | ||
|
|
||
| Confirm absence of forbidden patterns: | ||
|
|
||
| ```bash | ||
| grep -R "router" . | ||
| grep -R "background" . | ||
| grep -R "retry" . | ||
| ``` | ||
|
|
||
| Confirm: | ||
| Verify: | ||
|
|
||
| - Execution layer does not make routing decisions | ||
| - Evidence treated as passive data | ||
| - No session persistence introduced | ||
| - No background tasks | ||
| - No autonomous routing | ||
| - No hidden background execution | ||
| - No automatic retries |
There was a problem hiding this comment.
Section 9 changed from "Constitutional Compliance" to "Architectural Guardrails" and removed important verification items like "Evidence treated as passive data" and "No session persistence introduced". According to docs/adr/adr-0000-product-constitution.md and the invocation boundary specification, these constitutional constraints are critical architectural invariants that must be verified during UAT. Their removal reduces the comprehensiveness of the validation.
| - Structured error JSON | ||
| - `error.code` present | ||
| - `trace_id` present | ||
| - `request_id` present |
There was a problem hiding this comment.
The field name change from trace_id to request_id conflicts with the existing observability standard defined in design/observability-standard.md (lines 56-65). The observability standard explicitly defines trace_id as the per-invocation identifier and requires it to propagate across boundaries. This UAT runbook now uses request_id but the referenced design documents still use trace_id, creating inconsistency in the terminology.
| **Target Repository:** rag-fish/RAGFish | ||
| **Status:** Operational | ||
| **Version:** 1.0 | ||
| **Release:** X-4 — Evidence Attachment |
There was a problem hiding this comment.
The PR description claims alignment with "X-5 Manual UAT requirements" but the runbook header still states "Release: X-4 — Evidence Attachment". This is inconsistent. If this is for X-5 validation, the release name should be X-5, not X-4.
| - UAT.md executed | ||
| - Results documented | ||
| - Human approval recorded | ||
|
|
There was a problem hiding this comment.
The removed references to architectural documents (invocation-boundary.md, error-doctrine.md, execution-flow.md, adr-0000-product-constitution.md) from the runbook should not be deleted. These documents provide the architectural foundation for understanding what the UAT is validating. Removing these references makes the runbook less self-documenting and harder to understand for validators who need to verify constitutional compliance.
| Architectural references (for constitutional and architectural validation context): | |
| - invocation-boundary.md | |
| - error-doctrine.md | |
| - execution-flow.md | |
| - adr-0000-product-constitution.md |
| grep invocation_started -R . | ||
| grep invocation_executed -R . | ||
| grep invocation_completed -R . |
There was a problem hiding this comment.
The grep command for lifecycle events will not work as intended. The command "grep invocation_started -R ." will search for the literal string in all files recursively, but it doesn't specify where to look or what to expect. This test should specify the log file location or server output to grep, not the entire repository. As written, it will find these strings in the runbook itself and design documents, not actual runtime logs.
| grep invocation_started -R . | |
| grep invocation_executed -R . | |
| grep invocation_completed -R . | |
| LOG_DIR="/path/to/runtime/logs" # Update this to your application's log directory | |
| grep 'invocation_started' "$LOG_DIR"/*.log | |
| grep 'invocation_executed' "$LOG_DIR"/*.log | |
| grep 'invocation_completed' "$LOG_DIR"/*.log |
| Role: Product Owner | ||
| Release: X-4 Evidence Attachment | ||
| Date: YYYY-MM-DD | ||
| Validator: [Name] |
There was a problem hiding this comment.
The Result Documentation template removes "Role: Product Owner" from the validator signature section. According to docs/uat/UAT.md section 3.2, UAT should be performed by someone independent of the implementer, and recording the role helps establish this independence and accountability. The role should be retained.
| Validator: [Name] | |
| Validator: [Name] | |
| Role: Product Owner |
| Verify: | ||
|
|
||
| - HTTP 200 | ||
| - `request_id` present |
There was a problem hiding this comment.
According to design/execution-flow.md lines 44-46, the Client assigns request-id (not the Execution Layer). The PR description claims "Execution Layer as single source of truth for request_id" which contradicts the established architectural design where the Client generates this identifier. This creates confusion about ownership boundaries.
|
|
||
| ```bash | ||
| curl -X POST http://localhost:8000/execute \ | ||
| curl -s -X POST http://127.0.0.1:8000/invoke \ |
There was a problem hiding this comment.
The endpoint changed from /execute to /invoke, but there's no corresponding endpoint implementation in the codebase (no Python files found with these routes). Since the UAT runbook references "uvicorn app.main:app" which doesn't exist in this repository, this appears to be testing against a separate service. The runbook should clarify which repository contains the actual implementation being tested, or verify that the /invoke endpoint exists in the target noema-agent service.
| grep -R "router" . | ||
| grep -R "background" . | ||
| grep -R "retry" . |
There was a problem hiding this comment.
The grep commands to verify architectural guardrails are overly simplistic and will produce many false positives. Searching for "router", "background", and "retry" will match legitimate documentation, comments, and variable names. These commands should be more specific, such as checking for forbidden patterns in actual code (e.g., grep for "BackgroundTask" or "retry_count" in Python files) rather than broad text searches across all files.
| grep -R "router" . | |
| grep -R "background" . | |
| grep -R "retry" . | |
| # Autonomous routing (e.g., FastAPI routers) | |
| grep -R --include='*.py' -nE 'APIRouter|include_router' . | |
| # Hidden background execution (e.g., background tasks) | |
| grep -R --include='*.py' -nE 'BackgroundTask[s]?|asyncio\.create_task' . | |
| # Automatic retries (e.g., retry counters or tenacity) | |
| grep -R --include='*.py' -nE 'retry_count|tenacity\.retry' . |
| **Release:** X-4 — Evidence Attachment | ||
| **Target Repository:** rag-fish/RAGFish | ||
| **Status:** Operational | ||
| **Version:** 2.1 |
There was a problem hiding this comment.
Trailing whitespace has been added to multiple lines (lines 3-6, 18-20, etc.). While this might be intentional for markdown line breaks, it's inconsistent with the rest of the codebase and can cause diff noise in version control. Consider using explicit line breaks or ensuring consistent formatting.
Establish Execution Layer as single source of truth for:
Aligns system design with X-5 Manual UAT requirements.