Repository navigation
feat(record): preserve sourced documentary responses for approved brief items - #41
Merged
superWorldSavior merged 5 commits intoSep 13, 2026
Merged
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 48f2ec7c1b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical and moderate findings affect history integrity, storage routing, and closed-contract validation.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds an MRTR-authorized lifecycle for recording sourced documentary responses to approved brief items, preserving revisions without creating requirements or verification results.
Changes:
- Adds review, approval, CAS persistence, Thread succession, replay guards, and history validation.
- Exposes
project-response/2.0through MCP, native Workbench, and UI while preserving strict V1 history. - Registers operation metadata, documentation, schemas, and lifecycle tests.
File summaries
| File | Review summary |
|---|---|
src/ui/src/thread/types.ts |
Supports V1 and V2 response payloads. |
src/ui/src/project/overview.tsx |
Integrates V2 response props. |
src/ui/src/project/overview-response-index.tsx |
Renders documentary responses. |
src/ui/src/project/overview-response-index-model.ts |
Parses and models clause responses. Findings: moderate (3 votes) source-reference validation; moderate (1 vote) positive revisions; moderate (1 vote) strict brief identity validation. |
src/ui/overview-response-index-model_test.ts |
Tests V1/V2 parsing. |
src/tools/register_test.ts |
Updates tool inventory coverage. |
src/tools/project-control/project-response-tools.ts |
Adds V2 MCP output schemas. Finding: moderate (3 votes) schema must enforce source-reference discriminator fields. |
src/tools/project-control/project-response-tools_test.ts |
Tests MCP response output. |
src/tools/project-control/documentary-clause-response-review-tools.ts |
Adds the read-only review tool. |
src/tools/project-control/documentary-clause-response-review-tools_test.ts |
Tests review-tool behavior. |
src/tools/project-control.ts |
Registers project-control integration. |
src/testing/synthetic-approved-brief-baseline.ts |
Adds synthetic lifecycle fixtures. |
src/orchestration/operations/runtime-demand-registry_test.ts |
Verifies no runtime demand. |
src/orchestration/operations/registry.ts |
Registers the new operation. |
src/orchestration/operations/registry_test.ts |
Tests registry metadata. |
src/orchestration/operations/proposal-validation.ts |
Adds proposal grammar validation. |
src/orchestration/operations/proposal-validation_test.ts |
Tests proposal registration. |
src/orchestration/operations/path-lanes.ts |
Registers the operation path lane. |
src/orchestration/operations/operation-reference-docs_test.ts |
Pins operation documentation. |
src/domain/record/documentary-clause-response.ts |
Defines response contracts and parameter grammar. Finding: moderate (1 vote) unsupported parameter keys must be rejected. |
src/domain/record/documentary-clause-response_test.ts |
Tests response invariants. |
src/domain/project/project-response.ts |
Defines V1/V2 response models. Finding: moderate (1 vote) source references should be a discriminated union. |
src/domain/project/project-response_test.ts |
Tests response contracts. |
src/application/use-cases/project-response/read-project-response.ts |
Projects current and historical responses. |
src/application/use-cases/project-response/read-project-response_test.ts |
Tests response projection. |
src/application/ports/out/project-response/project-response-evidence-reader.ts |
Extends evidence-reader facts. |
src/application/ports/in/record/project-documentary-clause-response-review.ts |
Defines the review port. |
src/application/ports/in/project-response/project-response.ts |
Exposes response V2 contracts. |
src/adapters/thread/project-response-evidence-reader.ts |
Integrates documentary evidence into reads. |
src/adapters/shared/thread-write-basis-guard.ts |
Allows the new Thread writer. |
src/adapters/shared/thread-write-basis-guard_test.ts |
Tests write-basis registration. |
src/adapters/record/record-seal-documentary-clause-response-run-executor.ts |
Implements the registered writer. |
src/adapters/record/record-seal-documentary-clause-response-run-executor_test.ts |
Tests executor behavior. |
src/adapters/record/record-seal-documentary-clause-response-lifecycle_test.ts |
Tests end-to-end lifecycle behavior. |
src/adapters/record/documentary-clause-response-store.ts |
Provides documentary CAS storage. |
src/adapters/record/documentary-clause-response-inputs.ts |
Resolves exact sources and predecessors. |
src/adapters/record/documentary-clause-response-history.ts |
Reopens and validates response history. Findings: moderate (3 votes) exact source artifacts must be reopened and matched; critical (2 votes) deterministic claim IDs must be derived and checked. |
src/adapters/record/documentary-clause-response-brief.ts |
Reopens approved brief history. |
src/adapters/record/documentary-clause-response-approval.ts |
Validates human MRTR approval. |
src/adapters/record/documentary-clause-response-approval_test.ts |
Tests approval guards. |
src/adapters/record/capture-backed-documentary-clause-response-reviewer.ts |
Resolves source-backed review proposals. |
src/adapters/record/capture-backed-documentary-clause-response-reviewer_test.ts |
Tests review resolution. |
server.ts |
Composes storage, review, and execution dependencies. |
scripts/serve/serve-native-workbench.ts |
Composes native Workbench response reading. Finding: moderate (1 vote) reader and writer use different stores. |
scripts/serve/serve-native-workbench_test.ts |
Tests Workbench routing. |
docs/reference/codebase/project-thread-record.md |
Updates the record inventory. |
docs/reference/codebase/persistence-roots.md |
Documents persistence roots. |
docs/reference/agent/lookalike-traps.md |
Documents related operation distinctions. |
docs/reference/agent/agent-workspace.md |
Documents the operation and authority boundaries. |
docs/how-to/extend/README.md |
Updates the extension guide index. |
docs/how-to/extend/qualify-documentary-clause-response.md |
Documents the recording workflow. |
desktop/src/workbench/bff.ts |
Supplies snapshot access; no packaged BFF runtime adoption is included. |
Review details
Suppressed comments (5)
scripts/serve/serve-native-workbench.ts:1963
- The registered writer stores these captures under
state/local/documentary-clause-responses(server.ts:1429-1435), but the native Workbench points its reader atstate/local/requirements-captures/clause-responses. These are different stores, so a response sealed through normal project control will never appear in the native Workbench projection. Use the canonical documentary-response store (prefer a shared/injected directory) so the read surface consumes the records the writer publishes.
const documentaryClauseResponseCaptures = createDocumentaryClauseResponseStore(
`${REQUIREMENTS_CAPTURE_DESCRIPTOR.directory}/clause-responses`,
);
src/domain/project/project-response.ts:102
- This new source-reference type permits all combinations of fields because both
artifactIdanduriare optional, so TypeScript callers can construct athread-artifactwithout an artifact ID or anagent-resourcewithout a URI. Model the two variants as a discriminated union (matching the input grammar) so the domain contract cannot represent invalid source provenance.
export interface ProjectResponseClauseSourceRef {
readonly kind: "agent-resource" | "thread-artifact";
readonly artifactId?: string;
readonly uri?: string;
}
src/domain/record/documentary-clause-response.ts:872
- After checking only duplicates and units, this loop inserts every parameter key into the map and silently ignores keys that the parser never reads. That allows an approved proposal to carry arbitrary unsupported fields while still being accepted, despite this operation's closed parameter grammar; reject keys outside the exact base, source, and predecessor key sets and add an unknown-key case.
src/ui/src/project/overview-response-index-model.ts:547 Number.isIntegeraccepts zero and negative values here, while the shared project-response contract parses both artifact/brief revisions withpositiveInteger. A malformed V2 payload can therefore be reported as available and rendered with an invalid revision; require a positive revision for the new clause-response fields, includingsourceBrief.revision.
src/ui/src/project/overview-response-index-model.ts:565- The nested
sourceBriefidentity is hand-parsed as merely non-empty strings and an integer, unlikeparseProjectResponseBriefIdentity, which rejects unsafe IDs and thelatestalias and requires a positive revision. A malformed V2 payload can therefore remainavailablewith an invalid brief identity; reuse the shared identity parser or apply the same closed checks here.
- Files reviewed: 51/52 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
superWorldSavior
force-pushed
the
codex/documentary-clause-responses-20260913
branch
from
September 13, 2026 10:47
48f2ec7 to
896c86a
Compare
Owner
Author
|
All four Codex inline and four Copilot inline findings, plus all five suppressed Copilot observations, are addressed in 896c86a. Shared CAS defaults, closed signed parameters, early duplicate/predecessor input refusal, deterministic claim identity and exact historical source reopening are covered. sourceRefs is a closed discriminated union throughout domain/port/projection/MCP/UI. UI revisions must be positive and both documentary and verification sourceBrief identities reuse the canonical parser. After integration and rebase onto main including #39: 84 focused documentary/reader/MCP/UI tests, one native Workbench CAS-root test and 36 cost-preview/registry tests passed; backend glob/UI checks and changed-source fmt/lint passed. A fresh Copilot assessment is requested on this integrity update. Fixtures exercise the real registered writer; no actual ID01 execution or approval/content acceptance is claimed. |
There was a problem hiding this comment.
🟡 Changes recommended
A critical snapshot-binding issue and several moderate history, provenance, and fixture-contract issues remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (7)
src/adapters/record/documentary-clause-response-history.ts:256
- The read-side history only reopens
thread-artifactsources;agent-resourcereferences are skipped, and the history dependencies provide no resource reopener. If the original agent-resource capture is missing or its bytes no longer match the signed reference, the documentary capture still projects as available because only the response CAS is reread. Reopen each agent resource on this path and surface an unavailable result when it cannot be reopened, with a regression test.
for (const source of capture.sources) {
if (source.kind !== "thread-artifact") continue;
src/application/use-cases/project-response/read-project-response.ts:435
- When an item is removed from the current approved brief,
projectClauseResponseshas already filtered its records out, so this path retains only an artifact id in a diagnostic. The answer, scope, and source references are therefore absent from both the V2 evidence model and its UI/MCP projections, contradicting the contract that old answers remain visible as historical. Preserve the historical record in a dedicated historical projection (while keeping it non-current) instead of collapsing it to this diagnostic.
return facts.clauseResponses
.filter((record) => !currentIds.has(record.sourceItemId))
.map((record) =>
diagnostic(
"clause-response.removed-item",
src/application/use-cases/project-response/read-project-response_test.ts:27
- These new fixtures are passed as
ProjectResponseEvidenceFacts["clauseResponses"], whose contract requiresrevision; omitting it makes the checked test fixture type-invalid and causesprojectClauseResponsesto emitrevision: undefined, which is not a valid V2 response. Addrevision: 1here and in the other clause-response fixtures in this file.
artifactId: "documentary-clause-response-exclusion",
src/application/use-cases/project-response/read-project-response_test.ts:66
- This fixture has the same missing required
revisionfield as the other newProjectResponseClauseResponseFactliterals in this test file. Without it the fixture is type-invalid and the projected V2 record has an undefined revision.
artifactId: "documentary-clause-response-success",
src/application/use-cases/project-response/read-project-response_test.ts:105
- This historical fixture also omits the required
revisionfield. It should provide the claim revision so the test exercises the same valid V2 shape that the production projection emits.
artifactId: "documentary-clause-response-exclusion-old",
src/application/use-cases/project-response/read-project-response_test.ts:145
- This forged-response fixture omits the required
revisionfield as well, so it does not conform toProjectResponseClauseResponseFactand would produce an invalid V2 response before the unavailable-path assertion is meaningful.
artifactId: "documentary-clause-response-forged",
src/ui/src/project/overview-response-index-model.ts:590
- The V2 parser is documented as strict and the public schema defines
predecessorArtifactIdas anID, but this branch silently drops a present non-string value and accepts an empty string. A malformed successor can therefore be accepted as if it had no predecessor instead of making the payload unreadable; validate the optional field as a non-empty string before the existingissues.lengthgate.
- Files reviewed: 52/53 changed files
- Comments generated: 3
- Review effort level: Lite
Owner
Author
|
This PR remains source work in progress after the second Copilot review on head Six confirmed issues remain before merge:
The four suppressed fixture observations claiming a missing revision are not applicable to this head: all four fixtures already provide CI run 34752749430 passed for this head. That source validation does not resolve the six review issues or establish a completed ID01 engineering/tender demonstration. No merge, release or runtime adoption is part of this cleanup. |
superWorldSavior
marked this pull request as draft
September 13, 2026 12:57
Owner
Author
|
The six confirmed issues from Copilot review 5190464227 are addressed in commit
The four suppressed missing-revision fixture remarks remain inapplicable to this head. Final local validation: 84 type-checked focused tests plus one executed native CAS-root test; backend glob/UI type checks, source formatting and lint pass. CI and fresh PR review are now pending. No ERPNext source or runtime change is included. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e6b8ebb340
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical replay and predecessor-validation issues, plus moderate evidence and schema-validation issues, remain.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (5)
src/adapters/record/documentary-clause-response-history.ts:168
- After the captured source refs are checked against
base, the result is only checked for the response artifact itself. A direct successor whose same-ID source artifact has a different fingerprint or producer can still passvalidateThreadSnapshot(result)and its consumption checks, so the reader would expose a response from evidence different from the signed basis. Compare each captured Thread source inresultwith the corresponding artifact inbase, as the requirements-trace history reader does.
await reopenExactCaptureSources(capture.sources, base, d.resources);
src/adapters/thread/project-response-evidence-reader.ts:167
- When the Thread already declares a documentary-response artifact, the history reader must reopen its CAS; returning an empty record set when
input.capturesis absent silently hides those persisted responses and allows the project-response projection to remainavailable. This is different from the explicit unavailable failures below for missing snapshot/resource stores. Detect declared documentary-response artifacts and returnclause-response.unavailablewhen their CAS dependency is not composed, while retaining the empty result only for Threads with no such artifacts.
if (!input.captures) return { records: [] };
src/tools/project-control/project-response-tools.ts:379
RESPONSE_ITEMreusesCLAUSE_RESPONSE, whosesourceStateenum includesremoved, so the public MCP schema accepts a removed answer inside a current brief item. Removed responses are supposed to be emitted only inhistoricalClauseResponses; define a current-item clause-response schema that excludesremovedand keep the broader schema for the historical array.
src/ui/src/project/overview-response-index-model.ts:685- Agent-resource references are only checked as non-empty strings here, so a payload such as
uri: "not-a-cas-uri"is accepted and rendered. That contradicts the exactAgentResourceReferenceURI contract and the UI's stated malformed-source rejection; validate the shared agent-resource URI pattern or full identity before returning this ref.
src/ui/src/project/overview-response-index-model.ts:492 - The V2 parser enforces
removed/historicalonly for the dedicated history array; this loop still acceptssourceState: "removed"on a current item'sclauseResponses. A malformed BFF payload can therefore render a retired answer as current instead of keeping the current/history projection separated; reject that state while parsing item responses.
- Files reviewed: 54/55 changed files
- Comments generated: 2
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
Three unresolved moderate findings remain in lineage validation, unavailable-state preservation, and removed-item history validation.
Review details
Suppressed comments (3)
src/adapters/record/documentary-clause-response-history.ts:64
- This read-side history check validates the supplied snapshots and their direct pointer but never proves that the current
threadis a continuous immutable Thread lineage. A disconnected or forked snapshot containing the same artifact can therefore be accepted as current evidence, even though the writer/input path already requiresassertThreadSnapshotLineageIntact. Perform that exact lineage check before reopening the records so malformed Thread history remains unavailable.
validateThreadSnapshot(thread);
src/adapters/thread/project-response-evidence-reader.ts:167
- When the Thread already declares a documentary-response artifact, an unavailable
clauseResponsesCAS dependency is treated as an empty history here.ReadProjectResponsethen projects the current model withoutclause-response.unavailable, silently dropping a persisted answer instead of preserving the required literal unavailable state. Inspect the Thread for the documentary-response URI before this early return (and similarly only fail on missing snapshot/resource dependencies when a candidate exists); return an explicit unavailable failure for a declared candidate.
if (!input.captures) return { records: [] };
src/ui/src/project/overview-response-index-model.ts:827
- The V2 parser only checks the
removed/historicalflags here, so it still accepts a response whosesourceItemIdis one of the currentitemsand will render that live response again under the removed-item history. This projection is supposed to contain only items absent from the current brief; reject whenitemscontainsparsed.sourceItemId(and add a regression for that cross-placement).
- Files reviewed: 54/55 changed files
- Comments generated: 0 new
- Review effort level: Lite
Owner
Author
|
All seven valid findings in Copilot review5190909039 are addressed in c6d7378.
85 focused type-checked tests and backend/UI/fmt/lint checks pass on the final source. Quality CI34760981866 is pending on this head. No ERPNext, provider or runtime change. |
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical and moderate findings remain in history validation, evidence reopening, and UI history parsing.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
src/adapters/thread/project-response-evidence-reader.ts:188
- This check runs even when the current Thread declares no documentary-response artifact. A composition that supplies the optional clause-response CAS but not the optional snapshot/resource reopeners therefore marks an ordinary project response unavailable instead of returning an empty documentary history; the
!capturesbranch already avoids this case. Gate these requirements on an actually declared documentary-response record.
if (!input.snapshots) {
return {
records: [],
failure: {
status: "unavailable",
src/ui/src/project/overview-response-index-model.ts:839
- The historical array is intended to contain only responses for brief items removed from the current brief, but this parser checks only the state labels. It will therefore accept and render a response whose
sourceItemIdis one of the current item IDs under the removed-item history section. Reject historical entries whose source item matches any parsed current item before exposing this projection.
- Files reviewed: 54/55 changed files
- Comments generated: 3
- Review effort level: Lite
Comment on lines
+98
to
+103
| deterministicJson(artifact.inputArtifactIds) !== deterministicJson(inputIds) | ||
| ) { | ||
| reject( | ||
| "A clause-response artifact does not equal its sealed producer, time, identity and inputs.", | ||
| ); | ||
| } |
Comment on lines
+150
to
+164
| const base = await d.snapshots.get(capture.basis.snapshotId); | ||
| const result = await d.snapshots.get(run.resultSnapshot.snapshotId); | ||
| if ( | ||
| !base || base.id !== capture.basis.snapshotId || | ||
| base.revision !== capture.basis.revision || | ||
| base.subject.id !== capture.basis.subjectId || !result || | ||
| result.id !== run.resultSnapshot.snapshotId || | ||
| result.revision !== base.revision + 1 || | ||
| result.revision !== run.resultSnapshot.revision || | ||
| result.subject.id !== base.subject.id || | ||
| run.resultSnapshot.subjectId !== base.subject.id || | ||
| result.previous?.snapshotId !== base.id || | ||
| result.previous.revision !== base.revision | ||
| ) { | ||
| reject("The clause-response's exact direct Thread successor is unavailable."); |
| await reopenExactCaptureSources(threadInputs, base, d.resources); | ||
| for (const id of inputIds) { | ||
| const historical = base.artifacts.find((item) => item.id === id); | ||
| const published = result.artifacts.find((item) => item.id === id); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Brief items such as exclusions and context have no registered path to record a sourced answer and its later revisions. This PR adds
project_documentary_clause_response_reviewandrecord.seal-documentary-clause-response@1. The server resolves the exact approved brief item and captured sources; a signed human MRTR authorizes recording the agent-authored documentary proposal. Recording creates no Requirement, verification pass or acceptance of the answer content.The writer reuses project commands, the run lease, immutable CAS and a direct Thread successor. Readers reconstruct canonical records from the signed MRTR, historical approved brief, completed run and published successor. Every Thread input, including the predecessor, must be active on the signed basis and preserved unchanged in the published successor. Every agent resource is reopened through the existing
AgentResourceExactReopenerport. Missing or inconsistent evidence, including an uncomposed CAS for already declared documentary records, remains explicitly unavailable. Replay binds the receipt to its original immutable project revision and complete Thread result identity, and reconstructs approval, inputs and attachment from that historical project/run, including after later brief revisions.The current read contract is closed
project-response/2.0; historical/1.0remains literal and strict. Superseded responses remain historical. Responses for removed brief items retain answer, scope, source references and original brief identity in the dedicated non-currenthistoricalClauseResponsesprojection. The MCP summary stays bounded to 8 KiB and reports their count; full evidence uses the exactexpectedBasis. The existing read-only DT viewer renders this history. Server, standalone Workbench and packaged desktop BFF share the existing capture/resource stores. Embedders can configuredocumentaryClauseResponseDirectoryfor a durable mount.Signed parameters reject unknown keys and duplicate/predecessor Thread inputs before MRTR. MCP and UI enforce 1–8 clause-response sources. The UI rejects malformed source references, predecessor identifiers and current records in the removed-item history projection. Each clause-response must match the brief item under which it is displayed. Current items exclude the removed state; retired answers appear only in the dedicated historical projection. Agent-resource URIs follow the existing shared identity contract.
Validation:
No live ID01 engineering qualification, provider execution, ERPNext change, dependency/version change or runtime adoption is included.
Closes #36.