v18 Continuum opening campaign#90
Closed
flyingrobots wants to merge 7 commits into
Closed
Conversation
📝 WalkthroughWalkthroughThis PR updates release-narrative documentation from v17 completion to v18 Continuum compatibility, introduces design specifications for v18 artifact interoperability, and implements the first Continuum artifact domain types and JSON ingestion infrastructure with validation and policy-gated admission. ChangesV18 Continuum Compatibility and Artifact Ingestion
🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
flyingrobots
changed the title
Release Preflight
If you tag this commit as |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
test/unit/domain/continuum/ContinuumArtifactIngestionPolicy.test.ts (1)
9-19: ⚡ Quick winExtract repeated descriptor literals into named test constants.
'receipt-family','0.1.0', and related literals are duplicated across the helper and test cases. Centralizing them improves drift resistance and aligns with policy.As per coding guidelines, "Avoid magic strings and numbers; use named constants instead".
Also applies to: 24-25, 31-32, 38-39, 45-46, 52-55
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/unit/domain/continuum/ContinuumArtifactIngestionPolicy.test.ts` around lines 9 - 19, The helper function makeDescriptor and related tests use repeated literal values (e.g., 'receipt-family', '0.1.0', schema path, generatedBy, artifactKind, targets, witnessScope) which should be extracted to named test constants; create top-level constants (e.g., TEST_FAMILY_ID, TEST_VERSION, TEST_SCHEMA_PATH, TEST_GENERATED_BY, TEST_ARTIFACT_KIND, TEST_TARGETS, TEST_WITNESS_SCOPE) and replace the hard-coded literals in makeDescriptor and all referenced test cases so they reference those constants, ensuring ContinuumArtifactDescriptor construction and all assertions use the shared names instead of magic strings/numbers.test/unit/domain/index.exports.test.ts (1)
270-277: ⚡ Quick winReplace inline descriptor literals with shared named constants in this test block.
The constructor payload currently hardcodes multiple domain literals, which makes future protocol updates more brittle.
As per coding guidelines, "Avoid magic strings and numbers; use named constants instead".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/unit/domain/index.exports.test.ts` around lines 270 - 277, The test creates a ContinuumArtifactDescriptor with many inline "magic" literals; extract those repeated domain values into shared named constants and use them in the constructor call instead of hardcoded strings. Define constants (e.g., RECEIPT_FAMILY = 'receipt-family', DESCRIPTOR_VERSION = '0.1.0', SOURCE_SCHEMA_PATH = '~/git/continuum/schemas/continuum-receipt-family.graphql', GENERATED_BY = 'wesley witness-continuum --scope receipt-family', ARTIFACT_KIND = 'continuum.family.fixture', AUTHORITY = 'generated-fixture', TARGETS_TYPESCRIPT = ['typescript']) near the top of the test file and replace the inline values passed into the ContinuumArtifactDescriptor instantiation (the descriptor variable) with those constants.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/domain/continuum/ContinuumArtifactIngestionPolicy.ts`:
- Around line 7-20: The current assertGeneratedAuthority throws
ContinuumArtifactAuthorityError for expected policy rejections; change it to
return an explicit domain result (e.g., a boolean or a typed
IngestionResult/AdmissionResult) instead of throwing, and update
ingest(descriptor: ContinuumArtifactDescriptor) to call the new
assertGeneratedAuthority return value and return a rejection result when
authority is not generated rather than letting an exception propagate; remove
the throw of ContinuumArtifactAuthorityError from assertGeneratedAuthority,
update the ingest return type to the chosen Result type, and adjust any callers
to handle the admission/rejection result deterministically.
In `@src/infrastructure/adapters/ContinuumArtifactJsonFileAdapter.ts`:
- Around line 26-30: The loadString method currently lets JSON.parse throw raw
SyntaxError; wrap the parse in a try/catch inside loadString, and on error throw
an AdapterValidationError describing "malformed JSON" (include the original
error message or set it as the cause) so callers get a consistent adapter error.
Locate loadString, the JSON.parse call and ensure you still call
parseDescriptorFields(parsed) and this.policy.ingest(new
ContinuumArtifactDescriptor(fields)) after successful parse; only convert parse
failures to AdapterValidationError.
---
Nitpick comments:
In `@test/unit/domain/continuum/ContinuumArtifactIngestionPolicy.test.ts`:
- Around line 9-19: The helper function makeDescriptor and related tests use
repeated literal values (e.g., 'receipt-family', '0.1.0', schema path,
generatedBy, artifactKind, targets, witnessScope) which should be extracted to
named test constants; create top-level constants (e.g., TEST_FAMILY_ID,
TEST_VERSION, TEST_SCHEMA_PATH, TEST_GENERATED_BY, TEST_ARTIFACT_KIND,
TEST_TARGETS, TEST_WITNESS_SCOPE) and replace the hard-coded literals in
makeDescriptor and all referenced test cases so they reference those constants,
ensuring ContinuumArtifactDescriptor construction and all assertions use the
shared names instead of magic strings/numbers.
In `@test/unit/domain/index.exports.test.ts`:
- Around line 270-277: The test creates a ContinuumArtifactDescriptor with many
inline "magic" literals; extract those repeated domain values into shared named
constants and use them in the constructor call instead of hardcoded strings.
Define constants (e.g., RECEIPT_FAMILY = 'receipt-family', DESCRIPTOR_VERSION =
'0.1.0', SOURCE_SCHEMA_PATH =
'~/git/continuum/schemas/continuum-receipt-family.graphql', GENERATED_BY =
'wesley witness-continuum --scope receipt-family', ARTIFACT_KIND =
'continuum.family.fixture', AUTHORITY = 'generated-fixture', TARGETS_TYPESCRIPT
= ['typescript']) near the top of the test file and replace the inline values
passed into the ContinuumArtifactDescriptor instantiation (the descriptor
variable) with those constants.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3148126d-d6fe-412f-bb23-24935bf6121f
⛔ Files ignored due to path filters (4)
docs/design/0124-v17-release-blocker-dag.dotis excluded by!**/*.dotdocs/design/0124-v17-release-blocker-dag.svgis excluded by!**/*.svgdocs/design/0124-v17-release-blocker-status.csvis excluded by!**/*.csvdocs/design/continuum-categories.pdfis excluded by!**/*.pdf
📒 Files selected for processing (29)
docs/BEARING.mddocs/VISION.mddocs/design/0124-v17-release-blocker-dag.mddocs/design/0145-push-pr-review-merge/push-pr-review-merge.mddocs/design/0146-v18-continuum-compatibility-charter/v18-continuum-compatibility-charter.mddocs/design/0147-v18-continuum-contract-matrix/v18-continuum-contract-matrix.mddocs/design/0148-v18-warp-optic-realization-map/v18-warp-optic-realization-map.mddocs/design/0149-v18-continuum-artifact-ingestion/v18-continuum-artifact-ingestion.mddocs/design/continuum-categories.texdocs/method/backlog/README.mddocs/method/backlog/WORKLOADS.mddocs/method/backlog/bad-code/RELEASE_TRIAGE.mddocs/method/backlog/v18.0.0/PROTO_echo-shaped-edge-records.mddocs/method/backlog/v18.0.0/PROTO_echo-shaped-node-records.mddocs/method/backlog/v18.0.0/README.mddocs/method/retro/0145-push-pr-review-merge/push-pr-review-merge.mdindex.tssrc/domain/continuum/ContinuumArtifactAuthority.tssrc/domain/continuum/ContinuumArtifactDescriptor.tssrc/domain/continuum/ContinuumArtifactIngestionPolicy.tssrc/domain/continuum/ContinuumFamilyId.tssrc/domain/errors/ContinuumArtifactAuthorityError.tssrc/domain/errors/index.tssrc/infrastructure/adapters/ContinuumArtifactJsonFileAdapter.tstest/fixtures/continuum/receipt-family-generated-artifact.jsontest/unit/domain/continuum/ContinuumArtifactIngestionPolicy.test.tstest/unit/domain/errors/index.test.tstest/unit/domain/index.exports.test.tstest/unit/infrastructure/adapters/ContinuumArtifactJsonFileAdapter.test.ts
Comment on lines
+7
to
+20
| ingest(descriptor: ContinuumArtifactDescriptor): ContinuumArtifactDescriptor { | ||
| this.assertGeneratedAuthority(descriptor); | ||
| return descriptor; | ||
| } | ||
|
|
||
| /** Rejects descriptors whose authority would make local mirrors canonical. */ | ||
| assertGeneratedAuthority(descriptor: ContinuumArtifactDescriptor): void { | ||
| if (descriptor.hasGeneratedAuthority()) { | ||
| return; | ||
| } | ||
| throw new ContinuumArtifactAuthorityError( | ||
| `Continuum family ${descriptor.familyId.toString()} must be loaded from a generated artifact or fixture, not ${descriptor.authority.toString()}`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Return ingestion rejection as a domain result instead of throwing.
Non-generated authority is an expected policy outcome, but this path throws. Please model it as an explicit return value so callers handle admission/rejection deterministically.
Proposed direction
+export type ContinuumArtifactIngestionResult =
+ | { readonly ok: true; readonly descriptor: ContinuumArtifactDescriptor }
+ | { readonly ok: false; readonly error: ContinuumArtifactAuthorityError };
+
export default class ContinuumArtifactIngestionPolicy {
- ingest(descriptor: ContinuumArtifactDescriptor): ContinuumArtifactDescriptor {
- this.assertGeneratedAuthority(descriptor);
- return descriptor;
- }
-
- assertGeneratedAuthority(descriptor: ContinuumArtifactDescriptor): void {
- if (descriptor.hasGeneratedAuthority()) {
- return;
- }
- throw new ContinuumArtifactAuthorityError(
- `Continuum family ${descriptor.familyId.toString()} must be loaded from a generated artifact or fixture, not ${descriptor.authority.toString()}`,
- );
+ ingest(descriptor: ContinuumArtifactDescriptor): ContinuumArtifactIngestionResult {
+ if (descriptor.hasGeneratedAuthority()) {
+ return { ok: true, descriptor };
+ }
+ return {
+ ok: false,
+ error: new ContinuumArtifactAuthorityError(
+ `Continuum family ${descriptor.familyId.toString()} must be loaded from a generated artifact or fixture, not ${descriptor.authority.toString()}`,
+ ),
+ };
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/domain/continuum/ContinuumArtifactIngestionPolicy.ts` around lines 7 -
20, The current assertGeneratedAuthority throws ContinuumArtifactAuthorityError
for expected policy rejections; change it to return an explicit domain result
(e.g., a boolean or a typed IngestionResult/AdmissionResult) instead of
throwing, and update ingest(descriptor: ContinuumArtifactDescriptor) to call the
new assertGeneratedAuthority return value and return a rejection result when
authority is not generated rather than letting an exception propagate; remove
the throw of ContinuumArtifactAuthorityError from assertGeneratedAuthority,
update the ingest return type to the chosen Result type, and adjust any callers
to handle the admission/rejection result deterministically.
Comment on lines
+26
to
+30
| loadString(raw: string): ContinuumArtifactDescriptor { | ||
| const parsed: unknown = JSON.parse(raw); | ||
| const fields = parseDescriptorFields(parsed); | ||
| return this.policy.ingest(new ContinuumArtifactDescriptor(fields)); | ||
| } |
There was a problem hiding this comment.
Wrap malformed JSON parse failures as AdapterValidationError.
JSON.parse currently leaks raw SyntaxError, while the rest of the adapter uses AdapterValidationError. Normalizing this keeps one predictable error contract for invalid input.
Patch suggestion
loadString(raw: string): ContinuumArtifactDescriptor {
- const parsed: unknown = JSON.parse(raw);
+ let parsed: unknown;
+ try {
+ parsed = JSON.parse(raw);
+ } catch {
+ throw new AdapterValidationError('Continuum artifact descriptor JSON must be valid JSON');
+ }
const fields = parseDescriptorFields(parsed);
return this.policy.ingest(new ContinuumArtifactDescriptor(fields));
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/infrastructure/adapters/ContinuumArtifactJsonFileAdapter.ts` around lines
26 - 30, The loadString method currently lets JSON.parse throw raw SyntaxError;
wrap the parse in a try/catch inside loadString, and on error throw an
AdapterValidationError describing "malformed JSON" (include the original error
message or set it as the cause) so callers get a consistent adapter error.
Locate loadString, the JSON.parse call and ensure you still call
parseDescriptorFields(parsed) and this.policy.ingest(new
ContinuumArtifactDescriptor(fields)) after successful parse; only convert parse
failures to AdapterValidationError.
Member
Author
|
@codex please confirm these self-review findings before this PR proceeds. Self-Code Review Findings
Additional checks run during this review:
|
Member
Author
|
Superseded by #91 on branch |
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
1 participant
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.
Summary
Validation
Notes
This is opened as a draft PR because it is the first batch of the v18 campaign. BEARING now tracks slices 1-10 plus slice 11, and slices 1-5 are checked off with evidence in hand.
Summary by CodeRabbit
Documentation
New Features