feat: Milestone 3 - The Triage Agent (In Progress)#3
feat: Milestone 3 - The Triage Agent (In Progress)#3flyingrobots wants to merge 25 commits intofeat/milestone-2-heartbeatfrom
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing touches🧪 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 |
Resolved 36/37 review issues across 14 commits; v1.0.0-alpha.2. Guild terminology rename (Task→Quest), constructor validation, DI fixes, graceful daemon shutdown, safe prop extraction, tightened types, expanded test coverage (18 tests).
…codes Implement 31-test matrix covering all 13 patch invariants via single-fault mutation of a golden fixture. Fix 5 schema blockers for AJV strict mode, add machine-assertable InvariantCode enum, and refactor validator to return structured InvariantError objects.
Vitest shims __dirname but raw ESM execution would throw ReferenceError. Use fileURLToPath(import.meta.url) pattern. Also inline the test key constant to remove file dependency.
Move Ed25519 test seed to inline constant in helpers and create-fixture.ts. Add test/private.key to .gitignore. Also add .catch handler for unhandled promise rejection and use node: import prefix for consistency.
- Add SCHEMA member to InvariantCode enum; use it in formatAjvErrors - Comment explaining INV_007-010 gap (reserved for future DAG invariants) - Cache compiled AJV validator per schemaPath (avoid re-compilation) - Add keyringPath parameter to validatePatchOpsDocument API - Remove hardcoded dist/ path from isMain detection - Fix signPatch any type to Record<string, unknown> - Remove redundant Uint8Array.from wrapper in signPatchFixture - Type InvariantError.code as InvariantCode | string
- Add required: ["opType"] to all 9 operation if-clauses to prevent vacuous true evaluation when opType is absent - Clarify cardinality allOf as documentation-only (INV_001 enforced) - Use structuredClone in clonePatch helper (Node 22+) - Merge duplicate ### Added sections in CHANGELOG alpha.1 - Regenerate golden fixture for updated schema hash
…tion, type safety
flyingrobots
force-pushed
the
feat/milestone-3-triage
branch
from
426bb8c to
3664b59
Compare
|
Superseded by PR #4 which is now rebased on main with all feedback addressed. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
xyph-actuator.ts (1)
16-31:⚠️ Potential issue | 🟡 MinorInconsistent agent ID format.
Line 17 uses
'agent.prime'(dot separator) while line 76 uses'agent:prime'(colon separator). This inconsistency could cause claim verification failures if the IDs don't match during comparison.🔧 Proposed fix
async function getGraph(): Promise<WarpGraph> { - const writerId = process.env['XYPH_AGENT_ID'] || 'agent.prime'; + const writerId = process.env['XYPH_AGENT_ID'] || 'agent:prime'; // Every agent identifies as a unique writer in the XYPH roadmapsrc/domain/services/IngestService.ts (1)
1-29:⚠️ Potential issue | 🟡 MinorUpdate service comments to reflect Digital Guild terminology (Quests, Campaigns, Scrolls).
The
ingestMarkdownmethod parses raw sources into Task entities that represent Quests. Per CLAUDE.md and project coding guidelines, service documentation and variable names should use Digital Guild terminology. Update the JSDoc comments to reference "Quest entities" instead of "Task entities", and clarify the semantic mapping (Task.type='task' represents a Quest).Note: CONSTITUTION.md focuses on DAG integrity and does not mandate terminology; the Digital Guild vocabulary requirement comes from CLAUDE.md and the stated coding guidelines.
🤖 Fix all issues with AI agents
In @.coderabbit.yaml:
- Around line 5-17: The config uses unsupported keys: replace
reviews.target_branches with reviews.auto_review.base_branches (move the list
under the auto_review block), rename reviews.summary to
reviews.high_level_summary, and move reviews.chat.enabled up to a top-level
chat.enabled key; update the YAML to use reviews.auto_review.base_branches,
reviews.high_level_summary, and top-level chat.enabled so the settings match the
schema.
In `@src/domain/services/OrchestrationFSM.ts`:
- Around line 25-28: computeDigest currently calls JSON.stringify(data,
sortedKeys) which uses the replacer array and drops nested object keys; replace
this with a recursive canonicalization so nested keys are preserved — e.g., call
the existing canonicalize(data) (or implement a recursive key-sorting function)
to produce a deterministic JSON string and then hash that; update computeDigest
to use canonicalize(data) (or equivalent) before
crypto.createHash(...).update(...).digest(...) to avoid silent loss of nested
properties.
- Around line 34-49: transitionToNormalize must enforce that inputArtifact.state
=== 'INGEST' and compute a true outputDigest by hashing the complete output
artifact (everything except outputDigest) rather than using the placeholder;
update transitionToNormalize to validate the precondition (throw or return a
failed TransitionResult if inputArtifact.state !== 'INGEST'), build the
outputArtifact with schemaVersion, runId, state = 'NORMALIZE', createdAt,
inputDigest = inputArtifact.outputDigest, then call computeDigest on that output
object with outputDigest omitted (e.g., computeDigest({ schemaVersion, runId,
state, createdAt, inputDigest })) and set outputArtifact.outputDigest to that
real hash before returning; reference symbols: transitionToNormalize,
OrchestrationArtifact, inputArtifact.outputDigest, computeDigest, FSMContext,
OrchestrationState.
In `@src/validation/validatePatchOps.ts`:
- Around line 31-95: The code uses Task/Milestone/Patch/Operation domain names
(interfaces Operation, RollbackOperation, PatchOps and ORDERED_ENTITY_TYPES with
"TASK","MILESTONE","GRAPH_EDGE"); change these to Digital Guild terminology by
renaming types and enum values (e.g., Operation -> ScrollOperation or Operation
-> SealOperation per team convention, RollbackOperation -> ScrollRollback,
PatchOps -> CampaignPatch/CampaignOps) and replace the entityType literals
"TASK","MILESTONE","GRAPH_EDGE" with the required "Quest","Campaign","Seal" (or
create a clear mapping layer that translates between old schema values and new
code names to preserve payload/schema compatibility). Update
ORDERED_ENTITY_TYPES to use the Guild terms and update all references to the
interface names and entityType string checks throughout the file so invariants
and schemaVersion remain unchanged while code expresses
Quests/Campaigns/Scrolls/Seals.
In `@test/matrix/helpers/resignPatch.ts`:
- Around line 1-10: Remove the hardcoded TEST_PRIVATE_KEY constant in
resignPatch and instead load the key at runtime (e.g., from an environment
variable or a gitignored file) and fail fast if it's not present; update the
resignPatch function to read the key (and optionally TEST_KEY_ID) from
process.env or a file, validate the value is non-empty before calling
signPatch(patch, privateKey, keyId), and throw a clear error if the key is
missing so secret scanners and accidental commits are avoided.
In `@trust/keyring.json`:
- Around line 3-13: The default trust/keyring.json currently contains a test
signer (keyId "did:key:z6MkhTestSigner01") which lets anyone with that private
key create accepted patches; remove that test key from trust/keyring.json and
create a separate test-only keyring (e.g., test/keyring.test.json) containing
the test keys, update any test/config references to point to the new test
keyring, and ensure validatePatchOpsDocument continues to default to
trust/keyring.json for production use; search for references to the keyId
"did:key:z6MkhTestSigner01" and to validatePatchOpsDocument to update test
harnesses and CI to use the test keyring.
🟡 Minor comments (22)
test/unit/TriageService.test.ts-1-46 (1)
1-46:⚠️ Potential issue | 🟡 MinorAlign test terminology with Digital Guild conventions used elsewhere in tests.
The test uses
type: 'task'and references "backlog" rather than Digital Guild terminology (Quest, Campaign, Scroll, Seal) seen in other test files likeRebalanceService.test.ts. Per coding guidelines, this file must use Digital Guild terminology. Update test descriptions, task types, and IDs to match the pattern used in other tests (e.g.,type: 'campaign'ortype: 'scroll').scripts/setup-test-keys.ts-9-18 (1)
9-18:⚠️ Potential issue | 🟡 MinorRunning this script multiple times will create duplicate keyring entries.
The script appends a new key entry without checking if
testKeyIdalready exists. This could lead to duplicate entries intrust/keyring.jsonif the script is run multiple times.🛡️ Proposed fix to make the script idempotent
const keyring = JSON.parse(fs.readFileSync(keyringPath, 'utf8')); const testKeyId = 'did:key:z6MkhTestSigner01'; + + // Remove existing entry if present (idempotent) + keyring.keys = keyring.keys.filter((k: { keyId: string }) => k.keyId !== testKeyId); + keyring.keys.push({ keyId: testKeyId, alg: 'ed25519', publicKeyHex: keys.publicKeyHex });scripts/setup-test-keys.ts-24-24 (1)
24-24:⚠️ Potential issue | 🟡 MinorAdd error handling for the async setup function.
Unlike
scripts/create-fixture.tswhich has.catch()handling, this script's unhandled rejection could exit silently without a proper error message or exit code.🛡️ Proposed fix
-setup(); +setup().catch((err) => { + console.error('Failed to setup test keys:', err); + process.exit(1); +});scripts/verify-patch-ops.mjs-5-17 (1)
5-17:⚠️ Potential issue | 🟡 MinorUse Digital Guild terminology in CLI output.
User-facing log strings use “patch” wording; update to Quest/Campaign/Scroll/Seal terminology for consistency.
As per coding guidelines, Use 'Digital Guild' terminology (Quests, Campaigns, Scrolls, Seals) in code and documentation.
test/matrix/helpers/buildTwoOpPatch.ts-1-4 (1)
1-4:⚠️ Potential issue | 🟡 MinorAlign helper naming/value labels with Digital Guild lexicon.
Function and fixture strings use “Milestone” terminology; update to Quest/Campaign/Scroll/Seal terminology if these are user-facing fixtures.
As per coding guidelines, Use 'Digital Guild' terminology (Quests, Campaigns, Scrolls, Seals) in code and documentation.
test/integration/WarpRoadmapAdapter.test.ts-9-37 (1)
9-37:⚠️ Potential issue | 🟡 MinorUse Digital Guild terminology in test labels and data.
Test descriptions and sample data introduce generic “task” wording; adjust to Quest/Campaign/Scroll/Seal terminology where these labels are user-facing.
As per coding guidelines, Use 'Digital Guild' terminology (Quests, Campaigns, Scrolls, Seals) in code and documentation.
test/matrix/schema-boundary.test.ts-7-9 (1)
7-9:⚠️ Potential issue | 🟡 MinorAlign schema-boundary test labels with Digital Guild terminology.
The test descriptions use generic “schema/signature/patch” wording; update to Quest/Campaign/Scroll/Seal terminology where user-facing.
As per coding guidelines, Use 'Digital Guild' terminology (Quests, Campaigns, Scrolls, Seals) in code and documentation.
test/matrix/helpers/resignPatch.ts-3-4 (1)
3-4:⚠️ Potential issue | 🟡 MinorUse Digital Guild terminology in helper comments/naming.
The helper comment uses generic crypto wording; adjust to the required lexicon (e.g., Seal for signatures) where user-facing.
As per coding guidelines, Use 'Digital Guild' terminology (Quests, Campaigns, Scrolls, Seals) in code and documentation.
src/validation/InvariantError.ts-1-5 (1)
1-5:⚠️ Potential issue | 🟡 MinorAlign public error naming with Digital Guild terminology.
This exported interface introduces a new public type but doesn't reflect the required lexicon. Consider renaming or adding a Digital Guild alias (e.g.,
SealError) so downstream code/docs comply.As per coding guidelines, Use 'Digital Guild' terminology (Quests, Campaigns, Scrolls, Seals) in code and documentation.✏️ Suggested alias to align terminology
export interface InvariantError { code: InvariantCode | string; message: string; } + +// Digital Guild terminology alias for public API consistency. +export type SealError = InvariantError;scripts/verify-patch-ops.mjs-28-33 (1)
28-33:⚠️ Potential issue | 🟡 MinorRender validation errors with code/message instead of [object Object].
Interpolating error objects hides details. Format structured errors for readable CLI output.
🛠️ Suggested formatting for structured errors
- for (const e of result.errors) { - console.error(` - ${e}`); - } + for (const e of result.errors) { + if (typeof e === "string") { + console.error(` - ${e}`); + } else { + console.error(` - [${e.code}] ${e.message}`); + } + }test/integration/WarpRoadmapAdapter.test.ts-58-72 (1)
58-72:⚠️ Potential issue | 🟡 MinorMake the second test independent of the first.
It currently relies on data created in the previous test (Line 70), which is order-dependent. Create the baseline task in this test or in shared setup.
✅ One way to make the test self-contained
it('should return multiple tasks', async () => { const adapter = new WarpRoadmapAdapter(repoPath, graphName, writerId); + await adapter.upsertTask(new Task({ + id: 'task:INT-001', + title: 'Integration Task', + status: 'BACKLOG', + hours: 4, + type: 'task', + originContext: 'intent-123' + })); + await adapter.upsertTask(new Task({ id: 'task:INT-002', title: 'Task 2', status: 'BACKLOG', hours: 2, type: 'task' })); const tasks = await adapter.getTasks(); - // includes task:INT-001 from previous test because same repo/graph expect(tasks.length).toBeGreaterThanOrEqual(2); expect(tasks.some(t => t.id === 'task:INT-002')).toBe(true); });test/matrix/helpers/buildTwoOpPatch.ts-61-113 (1)
61-113:⚠️ Potential issue | 🟡 MinorReplace test fixture terminology with Digital Guild vocabulary.
buildTwoOpPatch()uses generic "MILESTONE" and "Alpha/Beta Milestone" terminology. Per coding guidelines, test fixtures matching**/*.tsmust use Digital Guild terminology (Quests, Campaigns, Scrolls, Seals). Update entity types and identifiers accordingly.Note: The rollbackOperations ordering (OP-0004, OP-0003) is correct—rollback operations are intentionally in reverse order to match the INV_002_REVERSE_MAP invariant, which validates the pairing of reverse-indexed operations to forward-indexed rollback entries.
docs/canonical/PATCH_OPS_SCHEMA.json-420-520 (1)
420-520:⚠️ Potential issue | 🟡 MinorSchema terminology doesn’t use Digital Guild vocabulary.
Definitions like
taskEntity,milestoneEntity, and related enums should be aligned to Quests/Campaigns/Scrolls/Seals (or add a formal mapping section) to match project terminology requirements.As per coding guidelines, Use 'Digital Guild' terminology (Quests, Campaigns, Scrolls, Seals) in code and documentation.
test/matrix/invariants.test.ts-11-35 (1)
11-35:⚠️ Potential issue | 🟡 MinorUse Digital Guild terminology in test descriptions.
The suite headings still use generic “Invariant” wording; update describe/it labels to the Quests/Campaigns/Scrolls/Seals vocabulary.
As per coding guidelines, Use 'Digital Guild' terminology (Quests, Campaigns, Scrolls, Seals) in code and documentation.
test/matrix/signature.test.ts-14-30 (1)
14-30:⚠️ Potential issue | 🟡 MinorUse Digital Guild terminology in test descriptions.
The describe/it labels still use generic “Invariant” language; please rename them to the project’s Quests/Campaigns/Scrolls/Seals vocabulary for consistency.
As per coding guidelines, Use 'Digital Guild' terminology (Quests, Campaigns, Scrolls, Seals) in code and documentation.
src/domain/services/OrchestrationFSM.ts-17-21 (1)
17-21:⚠️ Potential issue | 🟡 MinorUpdate naming/comments to Digital Guild terminology.
Class/docs here still use generic orchestration language; please align naming/commentary to Quests/Campaigns/Scrolls/Seals vocabulary.
As per coding guidelines, Use 'Digital Guild' terminology (Quests, Campaigns, Scrolls, Seals) in code and documentation.
src/validation/signPatchFixture.ts-13-31 (1)
13-31:⚠️ Potential issue | 🟡 MinorRename public API/fields to Digital Guild terminology.
The exported
signPatchAPI andsignaturefield names do not use Quests/Campaigns/Scrolls/Seals vocabulary required for new code. Consider renaming or providing a domain‑terminology wrapper.As per coding guidelines, Use 'Digital Guild' terminology (Quests, Campaigns, Scrolls, Seals) in code and documentation.
src/validation/InvariantCode.ts-1-18 (1)
1-18:⚠️ Potential issue | 🟡 MinorAlign enum naming with Digital Guild terminology.
InvariantCodeand its members use generic naming; please align to the required Quests/Campaigns/Scrolls/Seals vocabulary for new public identifiers.As per coding guidelines, Use 'Digital Guild' terminology (Quests, Campaigns, Scrolls, Seals) in code and documentation.
docs/canonical/ORCHESTRATION_SPEC.md-314-330 (1)
314-330:⚠️ Potential issue | 🟡 MinorAudit record example doesn’t match the code contract.
The JSON example shows
actoras a string and includesviolations/warnings, whileAuditRecordinsrc/domain/entities/Orchestration.tsdefinesactoras an object and has noviolations/warningsfields. Please align the spec example or the type contract to avoid confusion.src/domain/entities/Orchestration.ts-6-45 (1)
6-45:⚠️ Potential issue | 🟡 MinorAlign orchestration domain type names with Digital Guild terminology.
OrchestrationState,AuditRecord, andOrchestrationArtifactdon’t use Quests/Campaigns/Scrolls/Seals vocabulary required for new public types.As per coding guidelines, Use 'Digital Guild' terminology (Quests, Campaigns, Scrolls, Seals) in code and documentation.
docs/canonical/ORCHESTRATION_SPEC.md-1-16 (1)
1-16:⚠️ Potential issue | 🟡 MinorUpdate documentation terminology to Digital Guild vocabulary.
The spec uses generic planning/orchestration phrasing; align the narrative to Quests/Campaigns/Scrolls/Seals terminology to meet project conventions.
As per coding guidelines, Use 'Digital Guild' terminology (Quests, Campaigns, Scrolls, Seals) in code and documentation.
docs/canonical/PATCH_OPS_SCHEMA.json-121-134 (1)
121-134:⚠️ Potential issue | 🟡 MinorSync keyId pattern in canonical docs with actual source schema.
The canonical docs file has
"pattern": "^KEY-[A-Z0-9]{6,24}$"but the actual source schema atschemas/PATCH_OPS_SCHEMA.v1.jsonpermits bothKEY-*anddid:key:z6*formats:"^(KEY-[A-Z0-9]{6,24}|did:key:z6[A-Za-z0-9]+)$". All fixtures and tests use thedid:key:z6*format. Update the canonical docs to reflect the correct, broader pattern.
🧹 Nitpick comments (8)
docs/canonical/APPLY_TRANSACTION_SPEC.md (1)
90-99: Consider using Digital Guild terminology.The operation types use
TASKandMILESTONE(e.g.,ADD_TASK,MOVE_TASK_MILESTONE). As per coding guidelines, the project uses "Digital Guild" terminology where Tasks are called "Quests" and Milestones are called "Campaigns."Consider whether these op types should be renamed (e.g.,
ADD_QUEST,MOVE_QUEST_CAMPAIGN) for consistency with the project's terminology, or document explicitly that the internal operation types use different naming than the user-facing concepts.test/matrix/helpers/buildLinkDependencyOp.ts (2)
7-10: Hardcoded opIds may cause conflicts in multi-operation patches.The
opIdvalues are hardcoded to"OP-0010"and"OP-0011". If this helper is called multiple times within the same test patch, duplicate opIds will result. Consider parameterizing the opId or accepting a base index.♻️ Proposed fix to accept opId parameter
export function buildLinkDependencyOp( from: string, - to: string + to: string, + opIdBase = 10 ): { op: Record<string, unknown>; rollback: Record<string, unknown> } { const edgeId = `EDGE-${from.replace("TASK-", "")}${to.replace("TASK-", "")}`; + const opId = `OP-${String(opIdBase).padStart(4, '0')}`; + const rollbackOpId = `OP-${String(opIdBase + 1).padStart(4, '0')}`; const op: Record<string, unknown> = { - opId: "OP-0010", + opId, opType: "LINK_DEPENDENCY", // ... }; const rollback: Record<string, unknown> = { - opId: "OP-0011", - revertsOpId: "OP-0010", + opId: rollbackOpId, + revertsOpId: opId, // ... };Also applies to: 29-31
5-5: Edge ID generation assumes specific input format.The
edgeIdconstruction assumes inputs always have the"TASK-"prefix. If a different format is passed, the replace won't match and the full string will be used, which may produce unexpected edge IDs.test/unit/RebalanceService.test.ts (2)
8-8: Remove unnecessaryasynckeyword.The test functions are marked
asyncbutvalidateCampaignis synchronous and noawaitis used. This is misleading.♻️ Proposed fix
- it('should detect when a campaign exceeds 160 hours', async () => { + it('should detect when a campaign exceeds 160 hours', () => {- it('should pass when a campaign is within limits', async () => { + it('should pass when a campaign is within limits', () => {Also applies to: 22-22
5-31: Consider adding boundary test cases.The tests cover over-limit and under-limit scenarios but miss important boundaries:
- Exactly 160 hours (boundary condition)
- Empty tasks array (0 hours)
💚 Proposed additional test cases
it('should pass when a campaign is exactly at the 160 hour limit', () => { const tasks = [ new Task({ id: 'task:1', title: 'Task A', status: 'BACKLOG', hours: 80, type: 'task' }), new Task({ id: 'task:2', title: 'Task B', status: 'BACKLOG', hours: 80, type: 'task' }), ]; const result = service.validateCampaign('campaign:EXACT', tasks); expect(result.valid).toBe(true); expect(result.totalHours).toBe(160); }); it('should pass with an empty task list', () => { const result = service.validateCampaign('campaign:EMPTY', []); expect(result.valid).toBe(true); expect(result.totalHours).toBe(0); });src/domain/services/TriageService.ts (2)
21-21: Consider replacingconsole.logwith a structured logger.Direct
console.logcalls make it harder to control log levels, filter output in production, or integrate with observability tooling. Consider injecting a logger or using a logging abstraction.Also applies to: 45-47
20-26: Consider validating contextHash format.The
contextHashparameter is documented as a "BLAKE3 hash" but no validation ensures it follows the expected pattern (e.g.,blake3:[a-f0-9]{64}). Invalid hashes could be persisted silently.🛡️ Proposed validation
public async linkIntent(taskId: string, contextHash: string): Promise<void> { + if (!/^blake3:[a-f0-9]{64}$/.test(contextHash)) { + throw new Error(`Invalid contextHash format: ${contextHash}`); + } console.log(chalk.cyan(`[Triage] Linking ${taskId} to intent ${contextHash}`));Dockerfile (1)
11-14: Consider using a generic test identity.Hardcoding a personal email in the Docker image works but could be replaced with a generic test identity (e.g.,
test@example.com/CI Test User) to avoid coupling the image to a specific individual.♻️ Suggested change
# Configure git for integration tests -RUN git config --global user.email "james@flyingrobots.dev" && \ - git config --global user.name "James Ross" && \ +RUN git config --global user.email "test@example.com" && \ + git config --global user.name "CI Test User" && \ git config --global init.defaultBranch main
| reviews: | ||
| # Enable reviews on PRs targeting any branch (useful for stacked PRs) | ||
| target_branches: | ||
| - "*" | ||
| # Auto-review all PRs | ||
| auto_review: | ||
| enabled: true | ||
| # High-level summary of the PR | ||
| summary: | ||
| enabled: true | ||
| # Chat with CodeRabbit | ||
| chat: | ||
| enabled: true |
There was a problem hiding this comment.
Config keys don’t match the provided schema.
reviews.target_branches, reviews.summary, and reviews.chat aren’t defined in the schema, so these settings will be ignored. Use reviews.auto_review.base_branches, reviews.high_level_summary, and top-level chat instead.
✅ Suggested fix
reviews:
- target_branches:
- - "*"
auto_review:
enabled: true
+ base_branches:
+ - ".*"
- summary:
- enabled: true
+ high_level_summary: true
- chat:
- enabled: true
+chat:
+ enabled: true📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| reviews: | |
| # Enable reviews on PRs targeting any branch (useful for stacked PRs) | |
| target_branches: | |
| - "*" | |
| # Auto-review all PRs | |
| auto_review: | |
| enabled: true | |
| # High-level summary of the PR | |
| summary: | |
| enabled: true | |
| # Chat with CodeRabbit | |
| chat: | |
| enabled: true | |
| reviews: | |
| # Auto-review all PRs | |
| auto_review: | |
| enabled: true | |
| base_branches: | |
| - ".*" | |
| # High-level summary of the PR | |
| high_level_summary: true | |
| chat: | |
| enabled: true |
🤖 Prompt for AI Agents
In @.coderabbit.yaml around lines 5 - 17, The config uses unsupported keys:
replace reviews.target_branches with reviews.auto_review.base_branches (move the
list under the auto_review block), rename reviews.summary to
reviews.high_level_summary, and move reviews.chat.enabled up to a top-level
chat.enabled key; update the YAML to use reviews.auto_review.base_branches,
reviews.high_level_summary, and top-level chat.enabled so the settings match the
schema.
| public static computeDigest(data: Record<string, unknown>): string { | ||
| const sortedKeys = Object.keys(data).sort(); | ||
| const json = JSON.stringify(data, sortedKeys); | ||
| return `sha256:${crypto.createHash('sha256').update(json).digest('hex')}`; |
There was a problem hiding this comment.
computeDigest drops nested keys due to JSON.stringify replacer.
JSON.stringify(data, sortedKeys) whitelists top‑level keys for all nested objects, silently omitting nested properties and causing digest collisions. Use a canonicalizer that sorts keys recursively without dropping them.
🛠️ Proposed fix (reuse canonicalize)
-import crypto from 'node:crypto';
+import crypto from 'node:crypto';
+import { canonicalize } from '../../validation/crypto.js';
@@
public static computeDigest(data: Record<string, unknown>): string {
- const sortedKeys = Object.keys(data).sort();
- const json = JSON.stringify(data, sortedKeys);
+ const json = canonicalize(data as any);
return `sha256:${crypto.createHash('sha256').update(json).digest('hex')}`;
}🤖 Prompt for AI Agents
In `@src/domain/services/OrchestrationFSM.ts` around lines 25 - 28, computeDigest
currently calls JSON.stringify(data, sortedKeys) which uses the replacer array
and drops nested object keys; replace this with a recursive canonicalization so
nested keys are preserved — e.g., call the existing canonicalize(data) (or
implement a recursive key-sorting function) to produce a deterministic JSON
string and then hash that; update computeDigest to use canonicalize(data) (or
equivalent) before crypto.createHash(...).update(...).digest(...) to avoid
silent loss of nested properties.
| public static transitionToNormalize( | ||
| inputArtifact: OrchestrationArtifact, | ||
| context: FSMContext, | ||
| decisionSummary: string, | ||
| durationMs: number | ||
| ): TransitionResult { | ||
| const nextState: OrchestrationState = 'NORMALIZE'; | ||
|
|
||
| const outputArtifact: OrchestrationArtifact = { | ||
| schemaVersion: 'v1.0', | ||
| runId: context.runId, | ||
| state: nextState, | ||
| createdAt: new Date().toISOString(), | ||
| inputDigest: inputArtifact.outputDigest, | ||
| outputDigest: this.computeDigest({ state: nextState, runId: context.runId }) // Placeholder | ||
| }; |
There was a problem hiding this comment.
Validate input state and compute a real outputDigest (not placeholder).
The transition should enforce the INGEST precondition and hash the full output artifact (excluding outputDigest) instead of a placeholder to keep the audit chain trustworthy.
🛠️ Proposed fix
public static transitionToNormalize(
inputArtifact: OrchestrationArtifact,
context: FSMContext,
decisionSummary: string,
durationMs: number
): TransitionResult {
+ if (inputArtifact.state !== 'INGEST') {
+ throw new Error(`Invalid transition from ${inputArtifact.state} to NORMALIZE`);
+ }
const nextState: OrchestrationState = 'NORMALIZE';
- const outputArtifact: OrchestrationArtifact = {
- schemaVersion: 'v1.0',
- runId: context.runId,
- state: nextState,
- createdAt: new Date().toISOString(),
- inputDigest: inputArtifact.outputDigest,
- outputDigest: this.computeDigest({ state: nextState, runId: context.runId }) // Placeholder
- };
+ const baseArtifact: Omit<OrchestrationArtifact, 'outputDigest'> = {
+ schemaVersion: 'v1.0',
+ runId: context.runId,
+ state: nextState,
+ createdAt: new Date().toISOString(),
+ inputDigest: inputArtifact.outputDigest
+ };
+ const outputArtifact: OrchestrationArtifact = {
+ ...baseArtifact,
+ outputDigest: this.computeDigest(baseArtifact)
+ };🤖 Prompt for AI Agents
In `@src/domain/services/OrchestrationFSM.ts` around lines 34 - 49,
transitionToNormalize must enforce that inputArtifact.state === 'INGEST' and
compute a true outputDigest by hashing the complete output artifact (everything
except outputDigest) rather than using the placeholder; update
transitionToNormalize to validate the precondition (throw or return a failed
TransitionResult if inputArtifact.state !== 'INGEST'), build the outputArtifact
with schemaVersion, runId, state = 'NORMALIZE', createdAt, inputDigest =
inputArtifact.outputDigest, then call computeDigest on that output object with
outputDigest omitted (e.g., computeDigest({ schemaVersion, runId, state,
createdAt, inputDigest })) and set outputArtifact.outputDigest to that real hash
before returning; reference symbols: transitionToNormalize,
OrchestrationArtifact, inputArtifact.outputDigest, computeDigest, FSMContext,
OrchestrationState.
| interface Operation { | ||
| opId: string; | ||
| opType: string; | ||
| phase: number; | ||
| entityType: string; | ||
| entityId: string; | ||
| path: string; | ||
| value?: Json; | ||
| edge?: { fromTaskId: string; toTaskId: string }; | ||
| precondition: { | ||
| exists: boolean; | ||
| expectedHash: string; | ||
| expectedStatus?: string; | ||
| }; | ||
| invertibility: { | ||
| inverseOpType: string; | ||
| inversePath: string; | ||
| inverseValue: Json; | ||
| inversePreconditionHash: string; | ||
| }; | ||
| rationale?: string; | ||
| } | ||
|
|
||
| interface RollbackOperation extends Operation { | ||
| revertsOpId: string; | ||
| } | ||
|
|
||
| interface PatchOps { | ||
| schemaVersion: "v1.0"; | ||
| patchId: string; | ||
| runId: string; | ||
| baseSnapshotDigest: string; | ||
| policyPackRef: string; | ||
| configRef: string; | ||
| operations: Operation[]; | ||
| rollbackOperations: RollbackOperation[]; | ||
| approvals: { | ||
| required: boolean; | ||
| satisfied: boolean; | ||
| approverIds: string[]; | ||
| approvalRef?: string; | ||
| }; | ||
| metadata: { | ||
| createdAt: string; | ||
| author: { type: "human" | "agent" | "service"; id: string }; | ||
| rationale: string; | ||
| idempotencyKey?: string; | ||
| }; | ||
| signature: { | ||
| alg: "ed25519"; | ||
| keyId: string; | ||
| payloadDigest: string; | ||
| sig: string; | ||
| }; | ||
| } | ||
|
|
||
| /** | ||
| * ============================================================ | ||
| * Constants | ||
| * ============================================================ | ||
| */ | ||
|
|
||
| const ORDERED_ENTITY_TYPES = ["GRAPH_EDGE", "MILESTONE", "TASK"] as const; | ||
|
|
||
| /** |
There was a problem hiding this comment.
Align domain naming with Digital Guild terminology.
This module introduces Task/Milestone/Patch/Operation identifiers and enums; the guideline requires Quests/Campaigns/Scrolls/Seals in code and documentation. Please rename or map these identifiers (and keep schema/invariants consistent).
As per coding guidelines, "Use 'Digital Guild' terminology (Quests, Campaigns, Scrolls, Seals) in code and documentation".
🤖 Prompt for AI Agents
In `@src/validation/validatePatchOps.ts` around lines 31 - 95, The code uses
Task/Milestone/Patch/Operation domain names (interfaces Operation,
RollbackOperation, PatchOps and ORDERED_ENTITY_TYPES with
"TASK","MILESTONE","GRAPH_EDGE"); change these to Digital Guild terminology by
renaming types and enum values (e.g., Operation -> ScrollOperation or Operation
-> SealOperation per team convention, RollbackOperation -> ScrollRollback,
PatchOps -> CampaignPatch/CampaignOps) and replace the entityType literals
"TASK","MILESTONE","GRAPH_EDGE" with the required "Quest","Campaign","Seal" (or
create a clear mapping layer that translates between old schema values and new
code names to preserve payload/schema compatibility). Update
ORDERED_ENTITY_TYPES to use the Guild terms and update all references to the
interface names and entityType string checks throughout the file so invariants
and schemaVersion remain unchanged while code expresses
Quests/Campaigns/Scrolls/Seals.
test/matrix/helpers/resignPatch.ts
Outdated
There was a problem hiding this comment.
Remove the hardcoded test private key from source.
Even if test-only, committing a private key will trip secret scanners and risks unintended reuse. Load it from a gitignored file or env var and fail fast if missing.
🔐 Proposed fix: load the test key from env or a gitignored file
-import { signPatch } from "../../../src/validation/signPatchFixture.js";
+import { signPatch } from "../../../src/validation/signPatchFixture.js";
+import fs from "node:fs";
+import path from "node:path";
@@
-const TEST_PRIVATE_KEY = "5a10d58f976775c62f0c8443ef14b5b831561e26648a06f2791ad88e236375c7";
+const TEST_PRIVATE_KEY =
+ process.env.XYPH_TEST_PRIVATE_KEY ??
+ (fs.existsSync(path.resolve(process.cwd(), "test/private.key"))
+ ? fs.readFileSync(path.resolve(process.cwd(), "test/private.key"), "utf8").trim()
+ : "");
+
+if (!TEST_PRIVATE_KEY) {
+ throw new Error("Missing test private key. Run scripts/setup-test-keys.ts or set XYPH_TEST_PRIVATE_KEY.");
+}🧰 Tools
🪛 Gitleaks (8.30.0)
[high] 4-4: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 5-5: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 7-7: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🤖 Prompt for AI Agents
In `@test/matrix/helpers/resignPatch.ts` around lines 1 - 10, Remove the hardcoded
TEST_PRIVATE_KEY constant in resignPatch and instead load the key at runtime
(e.g., from an environment variable or a gitignored file) and fail fast if it's
not present; update the resignPatch function to read the key (and optionally
TEST_KEY_ID) from process.env or a file, validate the value is non-empty before
calling signPatch(patch, privateKey, keyId), and throw a clear error if the key
is missing so secret scanners and accidental commits are avoided.
| "keys": [ | ||
| { | ||
| "keyId": "did:key:z6MkhExampleSigner01", | ||
| "alg": "ed25519", | ||
| "publicKeyHex": "9f4c8b2b4c8b9fd9f06c01d6c9f4d88f2b8f7b8f6fd2a5c6e11a6f6ef9a5e123" | ||
| }, | ||
| { | ||
| "keyId": "did:key:z6MkhTestSigner01", | ||
| "alg": "ed25519", | ||
| "publicKeyHex": "2577162074909c27c9c14979f2755816f649bd9ca42f140a37061bef25185375" | ||
| } |
There was a problem hiding this comment.
Keep test/example keys out of the default trust keyring.
validatePatchOpsDocument defaults to trust/keyring.json, so including a known test signer allows anyone with the private key to produce accepted patches. Split prod vs test keyrings and point tests/config to the test keyring.
🧹 Proposed change (move test key to a test-only keyring)
{
"keyId": "did:key:z6MkhExampleSigner01",
"alg": "ed25519",
"publicKeyHex": "9f4c8b2b4c8b9fd9f06c01d6c9f4d88f2b8f7b8f6fd2a5c6e11a6f6ef9a5e123"
- },
- {
- "keyId": "did:key:z6MkhTestSigner01",
- "alg": "ed25519",
- "publicKeyHex": "2577162074909c27c9c14979f2755816f649bd9ca42f140a37061bef25185375"
}🧰 Tools
🪛 Gitleaks (8.30.0)
[high] 10-10: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🤖 Prompt for AI Agents
In `@trust/keyring.json` around lines 3 - 13, The default trust/keyring.json
currently contains a test signer (keyId "did:key:z6MkhTestSigner01") which lets
anyone with that private key create accepted patches; remove that test key from
trust/keyring.json and create a separate test-only keyring (e.g.,
test/keyring.test.json) containing the test keys, update any test/config
references to point to the new test keyring, and ensure validatePatchOpsDocument
continues to default to trust/keyring.json for production use; search for
references to the keyId "did:key:z6MkhTestSigner01" and to
validatePatchOpsDocument to update test harnesses and CI to use the test
keyring.
|
@coderabbitai review please |
|
Tip For best results, initiate chat on the files or code changes.
Did you want me to review:
Please clarify and I'll proceed with the review accordingly. 🔍 |
1 participant
Initial implementation of the Triage Agent:
Summary by CodeRabbit
New Features
Documentation
Infrastructure