Build the agent-native action kernel#52
Merged
flyingrobots merged 22 commits intomainfrom Mar 14, 2026
Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (29)
📝 WalkthroughWalkthroughAdds a canonical AGENT_PROTOCOL spec and agent-native action kernel plus CLI; implements new agent services (briefing, context, recommender, submission, validator, executor), settlement gating and key policy, diagnostic/doctor tooling, traceability rollups, runtime-launch refactor, TUI/dashboard augmentations, many type and test additions. Changes
Sequence Diagram(s)sequenceDiagram
participant User as Agent CLI User
participant CLI as Agent CLI
participant Brief as AgentBriefingService
participant Snap as GraphContext
participant Rec as AgentRecommender
participant Val as AgentActionValidator
User->>CLI: briefing / next
CLI->>Brief: buildBriefing() / next()
Brief->>Snap: fetchSnapshot()
Snap-->>Brief: snapshot (quests, submissions, reviews)
Brief->>Rec: recommendForQuest(quest, readiness)
Rec->>Val: validate(actionRequest)
Val-->>Rec: ValidatedAssessment
Rec-->>Brief: AgentActionCandidate[]
Brief-->>CLI: AgentBriefing (identity, assignments, frontier, reviewQueue)
CLI-->>User: JSON / human output
sequenceDiagram
participant User as Agent CLI User
participant CLI as Agent CLI
participant Svc as AgentActionService
participant Val as AgentActionValidator
participant Graph as GraphPort
participant Gate as SettlementGateService
User->>CLI: act --kind=seal|merge --args...
CLI->>Svc: execute(AgentActionRequest)
Svc->>Val: validate(request)
Val->>Graph: fetchEntityDetail(targetId)
Val-->>Svc: ValidatedAssessment
alt action is seal or merge
Svc->>Gate: assessSettlementGate(detail, action)
Gate-->>Svc: SettlementGateAssessment
alt allowed
Svc->>Graph: mutate graph (decisions, seal/merge nodes)
Svc-->>CLI: AgentActionOutcome {success, patch, detail}
else blocked
Svc-->>CLI: AgentActionOutcome {failure, code, reason}
end
else other action
Svc->>Graph: mutate graph (comments, claims, submissions)
Svc-->>CLI: AgentActionOutcome {success, patch, detail}
end
CLI-->>User: JSON / human result
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests (beta)
|
![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: 6aeccea281
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+1681
to
+1682
| result: 'success', | ||
| patch: null, |
There was a problem hiding this comment.
If adapter.decide(...) throws after the git merge succeeds, this path still reports result: 'success' with patch: null. In agent act merge, that is surfaced as a successful action (including top-level success: true JSON), so automation can stop retrying even though the submission decision was never recorded in the graph and the submission remains unsettled. This should be reported as a failed/rejected outcome so callers can reconcile and retry the decision write.
Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment.
✅ Addressed in commits 40cf996 and 253702a. AgentActionService.executeMerge() now returns result: "partial-failure" when git merge succeeds but the authoritative merge decision cannot be recorded, and xyph act --json routes that outcome through the error envelope so automation sees a non-success result and can reconcile/retry. Added regression coverage in /Users/james/git/xyph/test/unit/AgentActionService.test.ts and /Users/james/git/xyph/test/unit/AgentCommands.test.ts.
Owner
Author
|
@coderabbitai review please 🥕 |
|
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/domain/services/SubmissionService.ts (1)
220-227:⚠️ Potential issue | 🟠 MajorReject explicit patchsets that are not current heads.
Membership alone is too weak here. A caller can pass a superseded patchset, satisfy the
APPROVEDcheck on the latest tip, and still merge older code.--patchsetshould only resolve among current heads.Proposed head-only validation
if (explicitPatchsetId) { - const belongs = patchsetRefs.some((p) => p.id === explicitPatchsetId); - if (!belongs) { + const patchsetIds = new Set(patchsetRefs.map((p) => p.id)); + const supersededIds = new Set( + patchsetRefs + .map((p) => p.supersedesId) + .filter((id): id is string => typeof id === 'string'), + ); + if (!patchsetIds.has(explicitPatchsetId)) { throw new Error( `[NOT_FOUND] Patchset ${explicitPatchsetId} does not belong to submission ${submissionId}` ); } + if (supersededIds.has(explicitPatchsetId)) { + throw new Error( + `[INVALID_ARG] Patchset ${explicitPatchsetId} is not a current head of submission ${submissionId}` + ); + } return { tipPatchsetId: explicitPatchsetId }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/domain/services/SubmissionService.ts` around lines 220 - 227, The current check only verifies that explicitPatchsetId exists in patchsetRefs, allowing superseded patchsets; update the validation in SubmissionService so explicitPatchsetId must be one of the current heads instead of any member. Locate the block using explicitPatchsetId, patchsetRefs and tipPatchsetId and change the membership test to filter patchsetRefs for head patchsets (e.g., entries indicating current heads such as p.isHead / p.head / p.is_tip) and then verify explicitPatchsetId is in that filtered set; if not, throw the same NOT_FOUND error for submissionId. Ensure the function still returns { tipPatchsetId: explicitPatchsetId } when the head-only check passes.
🧹 Nitpick comments (4)
src/domain/services/AgentActionService.ts (2)
990-991: Repeated instantiation of FsKeyringAdapter and GuildSealService.
FsKeyringAdapterandGuildSealServiceare instantiated multiple times acrossvalidateSeal,validateMerge,executeSeal, andexecuteMerge. Consider injecting these as constructor dependencies to improve testability and reduce redundant instantiation.Also applies to: 1103-1104
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/domain/services/AgentActionService.ts` around lines 990 - 991, Multiple methods (validateSeal, validateMerge, executeSeal, executeMerge) repeatedly instantiate FsKeyringAdapter and GuildSealService; refactor AgentActionService to accept these as constructor dependencies instead: add constructor parameters for a KeyringAdapter (or FsKeyringAdapter interface) and a GuildSealService, store them as class properties, and replace the inline new FsKeyringAdapter() and new GuildSealService(...) calls in validateSeal, validateMerge, executeSeal and executeMerge with the injected instances so tests can mock them and instantiation is centralized.
726-726: Consider injecting GitWorkspaceAdapter instead of instantiating with process.cwd().Instantiating
GitWorkspaceAdapterwithprocess.cwd()in the validation method couples this service to the process environment, making unit testing harder. Consider accepting aWorkspacePortvia the constructor or method parameter.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/domain/services/AgentActionService.ts` at line 726, The validation code currently instantiates GitWorkspaceAdapter with process.cwd() inside AgentActionService (the line "const workspace = new GitWorkspaceAdapter(process.cwd());"), which couples the service to the process environment; refactor AgentActionService to accept a WorkspacePort (or GitWorkspaceAdapter) via the constructor or as a method parameter and use that injected instance in the validation method instead of creating a new GitWorkspaceAdapter with process.cwd(), updating the constructor signature and all call sites that construct AgentActionService (or the validation invocations) to provide the workspace dependency so unit tests can supply a mock workspace.src/domain/services/SettlementGateService.ts (1)
105-106: Policy fallback may silently select unrelated policy.When
computed?.policyIddoesn't match any policy in the array, the code falls back todetail.policies[0]. This could inadvertently apply the wrong policy if:
- The computed policyId references a policy that was removed
- The policies array has multiple policies but none match
Consider logging or flagging when the fallback is used, or returning a specific assessment indicating policy mismatch.
🔧 Potential improvement
- const appliedPolicy = detail.policies.find((policy) => policy.id === computed?.policyId) - ?? detail.policies[0]; + const matchedPolicy = detail.policies.find((policy) => policy.id === computed?.policyId); + const appliedPolicy = matchedPolicy ?? detail.policies[0]; + const policyMismatch = computed?.policyId && !matchedPolicy && detail.policies.length > 0;Then include
policyMismatchin the assessment for observability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/domain/services/SettlementGateService.ts` around lines 105 - 106, The current selection of appliedPolicy uses a silent fallback to detail.policies[0] when computed?.policyId doesn't match; update the logic in SettlementGateService so that after computing appliedPolicy (using the existing find fallback), detect whether the found policy's id equals computed?.policyId and, if not, set a clear flag (e.g., policyMismatch = true) and include that flag in the returned assessment object and/or emit a log entry via the service logger; reference the appliedPolicy variable, computed?.policyId, detail.policies, and the assessment returned by the method to add this observability without changing the primary selection behavior.src/cli/runtimeEntry.ts (1)
21-36: Minor: Redundant type check on line 31.The
argvariable is already typed asstringfrom theargvarray, sotypeof arg === 'string'is always true. This check is unnecessary.🔧 Suggested simplification
- if (typeof arg === 'string' && arg.startsWith('--as=')) continue; + if (arg.startsWith('--as=')) continue;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/runtimeEntry.ts` around lines 21 - 36, The check `typeof arg === 'string'` inside countCommandArgs is redundant because argv elements are already strings; remove that conditional and simplify the '--as=' branch to just `arg.startsWith('--as=')` in the loop within the countCommandArgs function so the `if` reads only `if (arg.startsWith('--as=')) continue;`, keeping the other branches (`'--tui'` and `'--as'` handling with i += 1) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/domain/services/AgentActionService.ts`:
- Around line 1677-1700: The catch in executeMerge (inside AgentActionService)
returns result: 'success' on adapter.decide() failure which is misleading;
change the returned result to a distinct value such as 'partial-success' (or
another agreed enum/string) and ensure all code paths and consumers that inspect
AgentActionService.executeMerge results (including any callers that expect
result values and any types/enums) are updated to recognize the new value; keep
the existing details.partialFailure payload and warnings, update any type
definitions/interfaces for the result field (and tests) to include
'partial-success', and ensure downstream logic that previously treated 'success'
as fully successful now treats 'partial-success' appropriately.
In `@src/domain/services/AgentBriefingService.ts`:
- Around line 210-216: The comparator in AgentBriefingService that sorts
candidates currently checks sourcePriority before allowed, letting
blocked/unexecutable candidates outrank actionable ones; change the comparator
in the candidates.sort call so that Number(b.allowed) - Number(a.allowed) is
evaluated before sourcePriority(a.source) - sourcePriority(b.source) (i.e.,
check allowed first), preserving the remaining tie-breakers (kindPriority,
confidence, targetId) in the same order to ensure executable candidates are
ranked ahead of source affinity.
In `@src/domain/services/AgentContextService.ts`:
- Around line 93-111: The code currently calls fetchSnapshot() and
fetchEntityDetail() separately which can materialize different graph states;
change the flow to materialize once and derive both the entity detail and
dependency from that single snapshot: call
createGraphContext(this.graphPort).fetchSnapshot() once into snapshot, then
obtain the entity detail from that same snapshot (e.g., use or add a method like
fetchEntityDetailFromSnapshot(snapshot, id) or overload fetchEntityDetail to
accept snapshot) and then call buildAgentDependencyContext(snapshot, quest) and
readiness.assess(id, { transition: false }) as before so detail, readiness and
dependency are all computed from the same snapshot.
In `@src/domain/services/AgentSubmissionService.ts`:
- Around line 192-198: The code currently emits a kind:'review' next-step using
submission.tipPatchsetId ?? submission.id which is invalid when tipPatchsetId is
missing; update the logic in AgentSubmissionService (the block using
isReviewableByAgent and producing the review object) to only emit kind:'review'
when submission.tipPatchsetId is a string/exists and set targetId =
submission.tipPatchsetId; if tipPatchsetId is absent, return an alternative step
(e.g., kind:'inspect' or kind:'wait' with a descriptive reason) or use a
separate unsupported-state context target so downstream commands aren’t given
submission.id as a patchset target. Ensure you reference
submission.tipPatchsetId when gating the review and keep supportedByActionKernel
logic consistent (only true when tipPatchsetId is present).
In `@src/domain/services/SettlementKeyPolicy.ts`:
- Around line 24-35: The function missingSettlementKeyData returns a loose
Record<string, unknown>; define and use a concrete return type (e.g., export
type or interface MissingSettlementKeyData) with explicit properties: agentId:
string, action: 'seal' | 'merge', missing: 'guild-seal-private-key',
overrideEnvVar: string (the UNSIGNED_SCROLLS_OVERRIDE_ENV constant), and hint:
string, then change the function signature to return MissingSettlementKeyData
and keep the same object shape inside the function so callers get full type
safety.
In `@src/domain/services/SubmissionService.ts`:
- Around line 69-81: The code currently reuses effectiveVerdicts when
getSubmissionSubmittedBy(submissionId) returns null, which allows an
author-inclusive verdict to be used; instead, treat a missing submitter as "no
independent verdicts" (fail-closed). Change the logic around
submittedBy/effectiveVerdicts/filterIndependentVerdicts so that when submittedBy
is null you set independentVerdicts to an empty
Map<string,'approve'|'request-changes'|'comment'> (do not fall back to
effectiveVerdicts), ensuring computeStatus and validateReview operate on the
empty set of independent verdicts; update any related variable names
(effectiveVerdicts, independentVerdicts) accordingly.
In `@src/infrastructure/GraphContext.ts`:
- Around line 752-789: Before calling deriveCampaignStatusFromQuests and
overwriting campaign.status, capture whether the campaign was manually marked
DONE (e.g., const wasManuallyDone = campaign.status === 'DONE') and then use
that captured value when setting the manualComplete option passed into
computeCompletionSummary; update references to campaign.status only after you’ve
stored the manual flag so computeCompletionSummary receives the original manual
state rather than the derived status. Ensure changes touch the loop where
deriveCampaignStatusFromQuests, campaign.status assignment, and the
computeCompletionSummary call occur.
In `@src/tui/render-status.ts`:
- Around line 84-86: The Assigned cell is replaced by a lone "!" when
q.computedCompletion?.discrepancy is true; instead preserve the assignee and
show the warning alongside it. In render-status.ts where the tuple element uses
q.computedCompletion?.discrepancy ? style.styled(style.theme.semantic.warning,
'!') : (q.assignedTo ?? '—'), change the branch so that when discrepancy is
present you render both the styled warning and the assignee (e.g., prefix or
suffix the styled '!' to (q.assignedTo ?? '—')), or alternatively move the
warning into a dedicated column; update the expression that builds that cell
(referencing q.computedCompletion?.discrepancy, q.assignedTo, and style.styled)
so the owner remains visible whenever a discrepancy exists.
In `@test/unit/AgentCommands.test.ts`:
- Around line 57-74: The test fixture makeCtx currently suppresses type errors
by returning the object with "as unknown as CliContext"; remove that double cast
and make the function return a proper CliContext shape instead — keep the
function named makeCtx and the inner stubbed properties (ok, warn, muted, print,
fail, failWithData, jsonOut, graphPort, style, identity) but type the return as
CliContext and replace any broad casts with targeted "as never" on individual
nested stubs that genuinely cannot be fully typed; ensure fail keeps its
implementation signature (fail: vi.fn((msg: string) => { throw new Error(msg);
})) and adjust any missing fields so the compiler surfaces real drift rather
than being silenced by the unknown->CliContext cast.
---
Outside diff comments:
In `@src/domain/services/SubmissionService.ts`:
- Around line 220-227: The current check only verifies that explicitPatchsetId
exists in patchsetRefs, allowing superseded patchsets; update the validation in
SubmissionService so explicitPatchsetId must be one of the current heads instead
of any member. Locate the block using explicitPatchsetId, patchsetRefs and
tipPatchsetId and change the membership test to filter patchsetRefs for head
patchsets (e.g., entries indicating current heads such as p.isHead / p.head /
p.is_tip) and then verify explicitPatchsetId is in that filtered set; if not,
throw the same NOT_FOUND error for submissionId. Ensure the function still
returns { tipPatchsetId: explicitPatchsetId } when the head-only check passes.
---
Nitpick comments:
In `@src/cli/runtimeEntry.ts`:
- Around line 21-36: The check `typeof arg === 'string'` inside countCommandArgs
is redundant because argv elements are already strings; remove that conditional
and simplify the '--as=' branch to just `arg.startsWith('--as=')` in the loop
within the countCommandArgs function so the `if` reads only `if
(arg.startsWith('--as=')) continue;`, keeping the other branches (`'--tui'` and
`'--as'` handling with i += 1) unchanged.
In `@src/domain/services/AgentActionService.ts`:
- Around line 990-991: Multiple methods (validateSeal, validateMerge,
executeSeal, executeMerge) repeatedly instantiate FsKeyringAdapter and
GuildSealService; refactor AgentActionService to accept these as constructor
dependencies instead: add constructor parameters for a KeyringAdapter (or
FsKeyringAdapter interface) and a GuildSealService, store them as class
properties, and replace the inline new FsKeyringAdapter() and new
GuildSealService(...) calls in validateSeal, validateMerge, executeSeal and
executeMerge with the injected instances so tests can mock them and
instantiation is centralized.
- Line 726: The validation code currently instantiates GitWorkspaceAdapter with
process.cwd() inside AgentActionService (the line "const workspace = new
GitWorkspaceAdapter(process.cwd());"), which couples the service to the process
environment; refactor AgentActionService to accept a WorkspacePort (or
GitWorkspaceAdapter) via the constructor or as a method parameter and use that
injected instance in the validation method instead of creating a new
GitWorkspaceAdapter with process.cwd(), updating the constructor signature and
all call sites that construct AgentActionService (or the validation invocations)
to provide the workspace dependency so unit tests can supply a mock workspace.
In `@src/domain/services/SettlementGateService.ts`:
- Around line 105-106: The current selection of appliedPolicy uses a silent
fallback to detail.policies[0] when computed?.policyId doesn't match; update the
logic in SettlementGateService so that after computing appliedPolicy (using the
existing find fallback), detect whether the found policy's id equals
computed?.policyId and, if not, set a clear flag (e.g., policyMismatch = true)
and include that flag in the returned assessment object and/or emit a log entry
via the service logger; reference the appliedPolicy variable,
computed?.policyId, detail.policies, and the assessment returned by the method
to add this observability without changing the primary selection behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7eba2586-eafe-45db-99ac-e9a3c516312b
📒 Files selected for processing (48)
README.mddocs/CLI-plan.mddocs/PLAN.mddocs/TUI-plan.mddocs/canonical/AGENT_CHARTER.mddocs/canonical/AGENT_PROTOCOL.mddocs/canonical/ARCHITECTURE.mddocs/canonical/ROADMAP_PROTOCOL.mddocs/canonical/TRACEABILITY.mdsrc/cli/commands/agent.tssrc/cli/commands/artifact.tssrc/cli/commands/dashboard.tssrc/cli/commands/show.tssrc/cli/commands/submission.tssrc/cli/runtimeEntry.tssrc/domain/entities/Submission.tssrc/domain/models/dashboard.tssrc/domain/services/AgentActionService.tssrc/domain/services/AgentBriefingService.tssrc/domain/services/AgentContextService.tssrc/domain/services/AgentRecommender.tssrc/domain/services/AgentSubmissionService.tssrc/domain/services/ReadinessService.tssrc/domain/services/SettlementGateService.tssrc/domain/services/SettlementKeyPolicy.tssrc/domain/services/SubmissionService.tssrc/domain/services/TraceabilityAnalysis.tssrc/infrastructure/GraphContext.tssrc/infrastructure/adapters/GitWorkspaceAdapter.tssrc/infrastructure/adapters/WarpSubmissionAdapter.tssrc/ports/WorkspacePort.tssrc/tui/bijou/views/roadmap-view.tssrc/tui/render-status.tstest/integration/GraphContextEntityDetail.test.tstest/unit/AgentActionService.test.tstest/unit/AgentBriefingService.test.tstest/unit/AgentCommands.test.tstest/unit/AgentContextService.test.tstest/unit/AgentSubmissionService.test.tstest/unit/DashboardTraceCommand.test.tstest/unit/ReadinessService.test.tstest/unit/SettlementGateService.test.tstest/unit/SignedSettlementCommands.test.tstest/unit/SubmissionService.test.tstest/unit/TraceabilityAnalysis.test.tstest/unit/runtimeEntry.test.tsxyph-actuator.tsxyph.ts
Comment on lines
+210
to
+216
| candidates.sort((a, b) => | ||
| sourcePriority(a.source) - sourcePriority(b.source) || | ||
| Number(b.allowed) - Number(a.allowed) || | ||
| kindPriority(a.kind) - kindPriority(b.kind) || | ||
| b.confidence - a.confidence || | ||
| a.targetId.localeCompare(b.targetId) | ||
| ); |
There was a problem hiding this comment.
Sort executable candidates ahead of source affinity.
sourcePriority currently wins before allowed, so a blocked assignment/submission candidate can outrank an allowed frontier claim. With small limits, agent next can hide the only action the agent can actually execute.
Put actionable work first
candidates.sort((a, b) =>
- sourcePriority(a.source) - sourcePriority(b.source) ||
Number(b.allowed) - Number(a.allowed) ||
+ sourcePriority(a.source) - sourcePriority(b.source) ||
kindPriority(a.kind) - kindPriority(b.kind) ||
b.confidence - a.confidence ||
a.targetId.localeCompare(b.targetId)
);📝 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.
Suggested change
| candidates.sort((a, b) => | |
| sourcePriority(a.source) - sourcePriority(b.source) || | |
| Number(b.allowed) - Number(a.allowed) || | |
| kindPriority(a.kind) - kindPriority(b.kind) || | |
| b.confidence - a.confidence || | |
| a.targetId.localeCompare(b.targetId) | |
| ); | |
| candidates.sort((a, b) => | |
| Number(b.allowed) - Number(a.allowed) || | |
| sourcePriority(a.source) - sourcePriority(b.source) || | |
| kindPriority(a.kind) - kindPriority(b.kind) || | |
| b.confidence - a.confidence || | |
| a.targetId.localeCompare(b.targetId) | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/domain/services/AgentBriefingService.ts` around lines 210 - 216, The
comparator in AgentBriefingService that sorts candidates currently checks
sourcePriority before allowed, letting blocked/unexecutable candidates outrank
actionable ones; change the comparator in the candidates.sort call so that
Number(b.allowed) - Number(a.allowed) is evaluated before
sourcePriority(a.source) - sourcePriority(b.source) (i.e., check allowed first),
preserving the remaining tie-breakers (kindPriority, confidence, targetId) in
the same order to ensure executable candidates are ranked ahead of source
affinity.
Comment on lines
+93
to
+111
| const graphCtx = createGraphContext(this.graphPort); | ||
| const snapshot = await graphCtx.fetchSnapshot(); | ||
| const detail = await graphCtx.fetchEntityDetail(id); | ||
| if (!detail) { | ||
| return null; | ||
| } | ||
|
|
||
| if (!detail.questDetail) { | ||
| return { | ||
| detail, | ||
| readiness: null, | ||
| dependency: null, | ||
| recommendedActions: [], | ||
| }; | ||
| } | ||
|
|
||
| const quest = detail.questDetail.quest; | ||
| const readiness = await this.readiness.assess(id, { transition: false }); | ||
| const dependency = buildAgentDependencyContext(snapshot, quest); |
There was a problem hiding this comment.
This can combine two different graph states in one response.
fetchSnapshot() and fetchEntityDetail() each re-sync/materialize the graph. If the frontier advances between them, detail.questDetail is built from the newer snapshot while dependency is still computed from the older one, so the same context payload can disagree with itself. Please derive both from a single materialization/snapshot.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/domain/services/AgentContextService.ts` around lines 93 - 111, The code
currently calls fetchSnapshot() and fetchEntityDetail() separately which can
materialize different graph states; change the flow to materialize once and
derive both the entity detail and dependency from that single snapshot: call
createGraphContext(this.graphPort).fetchSnapshot() once into snapshot, then
obtain the entity detail from that same snapshot (e.g., use or add a method like
fetchEntityDetailFromSnapshot(snapshot, id) or overload fetchEntityDetail to
accept snapshot) and then call buildAgentDependencyContext(snapshot, quest) and
readiness.assess(id, { transition: false }) as before so detail, readiness and
dependency are all computed from the same snapshot.
Comment on lines
+192
to
+198
| if (isReviewableByAgent(submission, agentId)) { | ||
| return { | ||
| kind: 'review', | ||
| targetId: submission.tipPatchsetId ?? submission.id, | ||
| reason: 'Review the current tip patchset for this submission.', | ||
| supportedByActionKernel: typeof submission.tipPatchsetId === 'string', | ||
| }; |
There was a problem hiding this comment.
Don't emit a review next-step when there is no tip patchset.
Falling back to submission.id here bubbles into downstream hints like xyph act review submission:*, which cannot work because review targets patchsets. Return an inspect/wait step until a tip patchset exists, or use a separate context target for the unsupported state.
Guard the unsupported state explicitly
if (isReviewableByAgent(submission, agentId)) {
+ if (!submission.tipPatchsetId) {
+ return {
+ kind: 'inspect',
+ targetId: submission.questId,
+ reason: 'Submission is open, but no tip patchset is available yet.',
+ supportedByActionKernel: false,
+ };
+ }
return {
kind: 'review',
- targetId: submission.tipPatchsetId ?? submission.id,
+ targetId: submission.tipPatchsetId,
reason: 'Review the current tip patchset for this submission.',
- supportedByActionKernel: typeof submission.tipPatchsetId === 'string',
+ supportedByActionKernel: true,
};
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/domain/services/AgentSubmissionService.ts` around lines 192 - 198, The
code currently emits a kind:'review' next-step using submission.tipPatchsetId ??
submission.id which is invalid when tipPatchsetId is missing; update the logic
in AgentSubmissionService (the block using isReviewableByAgent and producing the
review object) to only emit kind:'review' when submission.tipPatchsetId is a
string/exists and set targetId = submission.tipPatchsetId; if tipPatchsetId is
absent, return an alternative step (e.g., kind:'inspect' or kind:'wait' with a
descriptive reason) or use a separate unsupported-state context target so
downstream commands aren’t given submission.id as a patchset target. Ensure you
reference submission.tipPatchsetId when gating the review and keep
supportedByActionKernel logic consistent (only true when tipPatchsetId is
present).
Comment on lines
+24
to
+35
| export function missingSettlementKeyData( | ||
| agentId: string, | ||
| action: 'seal' | 'merge', | ||
| ): Record<string, unknown> { | ||
| return { | ||
| agentId, | ||
| action, | ||
| missing: 'guild-seal-private-key', | ||
| overrideEnvVar: UNSIGNED_SCROLLS_OVERRIDE_ENV, | ||
| hint: `Run 'xyph-actuator generate-key' before '${action}', or set ${UNSIGNED_SCROLLS_OVERRIDE_ENV}=1 for dev/test only.`, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use a concrete return type instead of Record<string, unknown>.
missingSettlementKeyData currently exposes a wildcard shape, which weakens consumer type safety.
♻️ Proposed refactor
+export interface MissingSettlementKeyData {
+ agentId: string;
+ action: 'seal' | 'merge';
+ missing: 'guild-seal-private-key';
+ overrideEnvVar: typeof UNSIGNED_SCROLLS_OVERRIDE_ENV;
+ hint: string;
+}
+
export function missingSettlementKeyData(
agentId: string,
action: 'seal' | 'merge',
-): Record<string, unknown> {
+): MissingSettlementKeyData {
return {
agentId,
action,
missing: 'guild-seal-private-key',
overrideEnvVar: UNSIGNED_SCROLLS_OVERRIDE_ENV,
hint: `Run 'xyph-actuator generate-key' before '${action}', or set ${UNSIGNED_SCROLLS_OVERRIDE_ENV}=1 for dev/test only.`,
};
}As per coding guidelines **/*.{ts,tsx}: Never use any type, wildcard imports/types to dodge type safety, or cast to any or unknown to silence the compiler in TypeScript.
📝 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.
Suggested change
| export function missingSettlementKeyData( | |
| agentId: string, | |
| action: 'seal' | 'merge', | |
| ): Record<string, unknown> { | |
| return { | |
| agentId, | |
| action, | |
| missing: 'guild-seal-private-key', | |
| overrideEnvVar: UNSIGNED_SCROLLS_OVERRIDE_ENV, | |
| hint: `Run 'xyph-actuator generate-key' before '${action}', or set ${UNSIGNED_SCROLLS_OVERRIDE_ENV}=1 for dev/test only.`, | |
| }; | |
| } | |
| export interface MissingSettlementKeyData { | |
| agentId: string; | |
| action: 'seal' | 'merge'; | |
| missing: 'guild-seal-private-key'; | |
| overrideEnvVar: typeof UNSIGNED_SCROLLS_OVERRIDE_ENV; | |
| hint: string; | |
| } | |
| export function missingSettlementKeyData( | |
| agentId: string, | |
| action: 'seal' | 'merge', | |
| ): MissingSettlementKeyData { | |
| return { | |
| agentId, | |
| action, | |
| missing: 'guild-seal-private-key', | |
| overrideEnvVar: UNSIGNED_SCROLLS_OVERRIDE_ENV, | |
| hint: `Run 'xyph-actuator generate-key' before '${action}', or set ${UNSIGNED_SCROLLS_OVERRIDE_ENV}=1 for dev/test only.`, | |
| }; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/domain/services/SettlementKeyPolicy.ts` around lines 24 - 35, The
function missingSettlementKeyData returns a loose Record<string, unknown>;
define and use a concrete return type (e.g., export type or interface
MissingSettlementKeyData) with explicit properties: agentId: string, action:
'seal' | 'merge', missing: 'guild-seal-private-key', overrideEnvVar: string (the
UNSIGNED_SCROLLS_OVERRIDE_ENV constant), and hint: string, then change the
function signature to return MissingSettlementKeyData and keep the same object
shape inside the function so callers get full type safety.
Comment on lines
+69
to
+81
| const submittedBy = await this.read.getSubmissionSubmittedBy(submissionId); | ||
|
|
||
| let effectiveVerdicts = new Map<string, 'approve' | 'request-changes' | 'comment'>(); | ||
| if (tip) { | ||
| const reviews = await this.read.getReviewsForPatchset(tip.id); | ||
| effectiveVerdicts = computeEffectiveVerdicts(reviews); | ||
| } | ||
| const independentVerdicts = submittedBy | ||
| ? filterIndependentVerdicts(effectiveVerdicts, submittedBy) | ||
| : effectiveVerdicts; | ||
|
|
||
| const decisions = await this.read.getDecisionsForSubmission(submissionId); | ||
| return computeStatus({ decisions, effectiveVerdicts }); | ||
| return computeStatus({ decisions, effectiveVerdicts: independentVerdicts }); |
There was a problem hiding this comment.
Don't fall back to author-inclusive verdicts when the submitter cannot be resolved.
If getSubmissionSubmittedBy() returns null, this path reuses effectiveVerdicts, so a self-review can still make the submission APPROVED. That breaks the new independent-review gate and, in the same malformed state, also leaves validateReview() fail-open.
Proposed fail-closed change
- const independentVerdicts = submittedBy
- ? filterIndependentVerdicts(effectiveVerdicts, submittedBy)
- : effectiveVerdicts;
+ const independentVerdicts = submittedBy
+ ? filterIndependentVerdicts(effectiveVerdicts, submittedBy)
+ : new Map<string, 'approve' | 'request-changes' | 'comment'>();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/domain/services/SubmissionService.ts` around lines 69 - 81, The code
currently reuses effectiveVerdicts when getSubmissionSubmittedBy(submissionId)
returns null, which allows an author-inclusive verdict to be used; instead,
treat a missing submitter as "no independent verdicts" (fail-closed). Change the
logic around submittedBy/effectiveVerdicts/filterIndependentVerdicts so that
when submittedBy is null you set independentVerdicts to an empty
Map<string,'approve'|'request-changes'|'comment'> (do not fall back to
effectiveVerdicts), ensuring computeStatus and validateReview operate on the
empty set of independent verdicts; update any related variable names
(effectiveVerdicts, independentVerdicts) accordingly.
Comment on lines
+250
to
+317
| const coverage = computeCoverageRatio(criteria); | ||
| const verdicts = computeCriterionVerdicts(criteria); | ||
| const failingCriterionIds = verdicts | ||
| .filter((entry) => entry.verdict === 'FAILED') | ||
| .map((entry) => entry.id); | ||
| const linkedOnlyCriterionIds = verdicts | ||
| .filter((entry) => entry.verdict === 'LINKED') | ||
| .map((entry) => entry.id); | ||
| const missingCriterionIds = verdicts | ||
| .filter((entry) => entry.verdict === 'MISSING') | ||
| .map((entry) => entry.id); | ||
|
|
||
| const tracked = requirements.length > 0 || criteria.length > 0; | ||
| const policy = options?.policy; | ||
| const manualComplete = options?.manualComplete ?? false; | ||
| const criterionCount = criteria.length; | ||
| const requirementCount = requirements.length; | ||
|
|
||
| let complete = false; | ||
| let verdict: ComputedCompletionSummary['verdict'] = 'UNTRACKED'; | ||
|
|
||
| if (!tracked) { | ||
| verdict = 'UNTRACKED'; | ||
| complete = manualComplete; | ||
| } else if (criterionCount === 0) { | ||
| verdict = 'MISSING'; | ||
| } else if (failingCriterionIds.length > 0) { | ||
| verdict = 'FAILED'; | ||
| } else if (policy) { | ||
| const meetsCoverage = coverage.ratio >= policy.coverageThreshold; | ||
| const blocksForLinked = policy.requireAllCriteria && linkedOnlyCriterionIds.length > 0; | ||
| const blocksForMissing = (policy.requireAllCriteria || policy.requireEvidence) && missingCriterionIds.length > 0; | ||
|
|
||
| if (blocksForLinked) { | ||
| verdict = 'LINKED'; | ||
| } else if (blocksForMissing) { | ||
| verdict = 'MISSING'; | ||
| } else if (meetsCoverage) { | ||
| verdict = 'SATISFIED'; | ||
| complete = true; | ||
| } else if (linkedOnlyCriterionIds.length > 0) { | ||
| verdict = 'LINKED'; | ||
| } else { | ||
| verdict = 'MISSING'; | ||
| } | ||
| } else if (linkedOnlyCriterionIds.length > 0) { | ||
| verdict = 'LINKED'; | ||
| } else if (missingCriterionIds.length > 0) { | ||
| verdict = 'MISSING'; | ||
| } else { | ||
| verdict = 'SATISFIED'; | ||
| complete = true; | ||
| } | ||
|
|
||
| return { | ||
| tracked, | ||
| complete, | ||
| verdict, | ||
| requirementCount, | ||
| criterionCount, | ||
| coverageRatio: coverage.ratio, | ||
| satisfiedCount: coverage.satisfied, | ||
| failingCriterionIds, | ||
| linkedOnlyCriterionIds, | ||
| missingCriterionIds, | ||
| policyId: policy?.id, | ||
| discrepancy: tracked ? computeDiscrepancy(manualComplete, complete) : undefined, | ||
| }; |
There was a problem hiding this comment.
Treat requirement-declared but absent criteria as MISSING.
Unlike computeUnmetRequirements(), this summary only looks at the criteria entries that were materialized. If a requirement references criterion:B but that node was filtered out or malformed, we currently undercount criterionCount, leave missingCriterionIds empty, and can even return complete: true.
Proposed normalization before computing coverage/verdicts
- const coverage = computeCoverageRatio(criteria);
- const verdicts = computeCriterionVerdicts(criteria);
+ const criteriaById = new Map(criteria.map((criterion) => [criterion.id, criterion] as const));
+ for (const requirement of requirements) {
+ for (const criterionId of requirement.criterionIds) {
+ if (!criteriaById.has(criterionId)) {
+ criteriaById.set(criterionId, { id: criterionId, evidence: [] });
+ }
+ }
+ }
+ const normalizedCriteria = [...criteriaById.values()];
+ const coverage = computeCoverageRatio(normalizedCriteria);
+ const verdicts = computeCriterionVerdicts(normalizedCriteria);
@@
- const criterionCount = criteria.length;
+ const criterionCount = normalizedCriteria.length;Please add a regression case for a requirement that references a criterion ID missing from the criteria array.
📝 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.
Suggested change
| const coverage = computeCoverageRatio(criteria); | |
| const verdicts = computeCriterionVerdicts(criteria); | |
| const failingCriterionIds = verdicts | |
| .filter((entry) => entry.verdict === 'FAILED') | |
| .map((entry) => entry.id); | |
| const linkedOnlyCriterionIds = verdicts | |
| .filter((entry) => entry.verdict === 'LINKED') | |
| .map((entry) => entry.id); | |
| const missingCriterionIds = verdicts | |
| .filter((entry) => entry.verdict === 'MISSING') | |
| .map((entry) => entry.id); | |
| const tracked = requirements.length > 0 || criteria.length > 0; | |
| const policy = options?.policy; | |
| const manualComplete = options?.manualComplete ?? false; | |
| const criterionCount = criteria.length; | |
| const requirementCount = requirements.length; | |
| let complete = false; | |
| let verdict: ComputedCompletionSummary['verdict'] = 'UNTRACKED'; | |
| if (!tracked) { | |
| verdict = 'UNTRACKED'; | |
| complete = manualComplete; | |
| } else if (criterionCount === 0) { | |
| verdict = 'MISSING'; | |
| } else if (failingCriterionIds.length > 0) { | |
| verdict = 'FAILED'; | |
| } else if (policy) { | |
| const meetsCoverage = coverage.ratio >= policy.coverageThreshold; | |
| const blocksForLinked = policy.requireAllCriteria && linkedOnlyCriterionIds.length > 0; | |
| const blocksForMissing = (policy.requireAllCriteria || policy.requireEvidence) && missingCriterionIds.length > 0; | |
| if (blocksForLinked) { | |
| verdict = 'LINKED'; | |
| } else if (blocksForMissing) { | |
| verdict = 'MISSING'; | |
| } else if (meetsCoverage) { | |
| verdict = 'SATISFIED'; | |
| complete = true; | |
| } else if (linkedOnlyCriterionIds.length > 0) { | |
| verdict = 'LINKED'; | |
| } else { | |
| verdict = 'MISSING'; | |
| } | |
| } else if (linkedOnlyCriterionIds.length > 0) { | |
| verdict = 'LINKED'; | |
| } else if (missingCriterionIds.length > 0) { | |
| verdict = 'MISSING'; | |
| } else { | |
| verdict = 'SATISFIED'; | |
| complete = true; | |
| } | |
| return { | |
| tracked, | |
| complete, | |
| verdict, | |
| requirementCount, | |
| criterionCount, | |
| coverageRatio: coverage.ratio, | |
| satisfiedCount: coverage.satisfied, | |
| failingCriterionIds, | |
| linkedOnlyCriterionIds, | |
| missingCriterionIds, | |
| policyId: policy?.id, | |
| discrepancy: tracked ? computeDiscrepancy(manualComplete, complete) : undefined, | |
| }; | |
| const criteriaById = new Map(criteria.map((criterion) => [criterion.id, criterion] as const)); | |
| for (const requirement of requirements) { | |
| for (const criterionId of requirement.criterionIds) { | |
| if (!criteriaById.has(criterionId)) { | |
| criteriaById.set(criterionId, { id: criterionId, evidence: [] }); | |
| } | |
| } | |
| } | |
| const normalizedCriteria = [...criteriaById.values()]; | |
| const coverage = computeCoverageRatio(normalizedCriteria); | |
| const verdicts = computeCriterionVerdicts(normalizedCriteria); | |
| const failingCriterionIds = verdicts | |
| .filter((entry) => entry.verdict === 'FAILED') | |
| .map((entry) => entry.id); | |
| const linkedOnlyCriterionIds = verdicts | |
| .filter((entry) => entry.verdict === 'LINKED') | |
| .map((entry) => entry.id); | |
| const missingCriterionIds = verdicts | |
| .filter((entry) => entry.verdict === 'MISSING') | |
| .map((entry) => entry.id); | |
| const tracked = requirements.length > 0 || criteria.length > 0; | |
| const policy = options?.policy; | |
| const manualComplete = options?.manualComplete ?? false; | |
| const criterionCount = normalizedCriteria.length; | |
| const requirementCount = requirements.length; | |
| let complete = false; | |
| let verdict: ComputedCompletionSummary['verdict'] = 'UNTRACKED'; | |
| if (!tracked) { | |
| verdict = 'UNTRACKED'; | |
| complete = manualComplete; | |
| } else if (criterionCount === 0) { | |
| verdict = 'MISSING'; | |
| } else if (failingCriterionIds.length > 0) { | |
| verdict = 'FAILED'; | |
| } else if (policy) { | |
| const meetsCoverage = coverage.ratio >= policy.coverageThreshold; | |
| const blocksForLinked = policy.requireAllCriteria && linkedOnlyCriterionIds.length > 0; | |
| const blocksForMissing = (policy.requireAllCriteria || policy.requireEvidence) && missingCriterionIds.length > 0; | |
| if (blocksForLinked) { | |
| verdict = 'LINKED'; | |
| } else if (blocksForMissing) { | |
| verdict = 'MISSING'; | |
| } else if (meetsCoverage) { | |
| verdict = 'SATISFIED'; | |
| complete = true; | |
| } else if (linkedOnlyCriterionIds.length > 0) { | |
| verdict = 'LINKED'; | |
| } else { | |
| verdict = 'MISSING'; | |
| } | |
| } else if (linkedOnlyCriterionIds.length > 0) { | |
| verdict = 'LINKED'; | |
| } else if (missingCriterionIds.length > 0) { | |
| verdict = 'MISSING'; | |
| } else { | |
| verdict = 'SATISFIED'; | |
| complete = true; | |
| } | |
| return { | |
| tracked, | |
| complete, | |
| verdict, | |
| requirementCount, | |
| criterionCount, | |
| coverageRatio: coverage.ratio, | |
| satisfiedCount: coverage.satisfied, | |
| failingCriterionIds, | |
| linkedOnlyCriterionIds, | |
| missingCriterionIds, | |
| policyId: policy?.id, | |
| discrepancy: tracked ? computeDiscrepancy(manualComplete, complete) : undefined, | |
| }; |
Comment on lines
+752
to
+789
| for (const campaign of campaigns) { | ||
| const memberQuests = questsByCampaignId.get(campaign.id); | ||
| if (memberQuests && memberQuests.length > 0) { | ||
| campaign.status = deriveCampaignStatusFromQuests(memberQuests); | ||
| } | ||
|
|
||
| const questIds = new Set((memberQuests ?? []).map((quest) => quest.id)); | ||
| const campaignRequirements = requirements.filter((requirement) => requirement.taskIds.some((taskId) => questIds.has(taskId))); | ||
| const campaignCriteria = campaignRequirements.flatMap((requirement) => criteriaByRequirementId.get(requirement.id) ?? []); | ||
| const appliedPolicy = policyByCampaignId.get(campaign.id); | ||
| campaign.computedCompletion = computeCompletionSummary( | ||
| campaignRequirements.map((requirement) => ({ | ||
| id: requirement.id, | ||
| criterionIds: requirement.criterionIds, | ||
| })), | ||
| campaignCriteria.map((criterion) => ({ | ||
| id: criterion.id, | ||
| evidence: criterion.evidenceIds | ||
| .map((evidenceId) => evidence.find((entry) => entry.id === evidenceId)) | ||
| .filter((entry): entry is EvidenceNode => Boolean(entry)) | ||
| .map((entry) => ({ | ||
| id: entry.id, | ||
| result: entry.result, | ||
| producedAt: entry.producedAt, | ||
| })), | ||
| })), | ||
| { | ||
| policy: appliedPolicy | ||
| ? { | ||
| id: appliedPolicy.id, | ||
| coverageThreshold: appliedPolicy.coverageThreshold, | ||
| requireAllCriteria: appliedPolicy.requireAllCriteria, | ||
| requireEvidence: appliedPolicy.requireEvidence, | ||
| } | ||
| : undefined, | ||
| manualComplete: campaign.status === 'DONE', | ||
| }, | ||
| ); |
There was a problem hiding this comment.
Capture manual campaign completion before overwriting campaign.status.
After the derived rollup runs, campaign.status is no longer the graph's manual status. A manually DONE campaign with incomplete member quests therefore loses its MANUAL_DONE_BUT_COMPUTED_INCOMPLETE discrepancy entirely.
Proposed fix
for (const campaign of campaigns) {
+ const manualComplete = campaign.status === 'DONE';
const memberQuests = questsByCampaignId.get(campaign.id);
if (memberQuests && memberQuests.length > 0) {
campaign.status = deriveCampaignStatusFromQuests(memberQuests);
}
@@
{
policy: appliedPolicy
? {
id: appliedPolicy.id,
coverageThreshold: appliedPolicy.coverageThreshold,
requireAllCriteria: appliedPolicy.requireAllCriteria,
requireEvidence: appliedPolicy.requireEvidence,
}
: undefined,
- manualComplete: campaign.status === 'DONE',
+ manualComplete,
},
);
}📝 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.
Suggested change
| for (const campaign of campaigns) { | |
| const memberQuests = questsByCampaignId.get(campaign.id); | |
| if (memberQuests && memberQuests.length > 0) { | |
| campaign.status = deriveCampaignStatusFromQuests(memberQuests); | |
| } | |
| const questIds = new Set((memberQuests ?? []).map((quest) => quest.id)); | |
| const campaignRequirements = requirements.filter((requirement) => requirement.taskIds.some((taskId) => questIds.has(taskId))); | |
| const campaignCriteria = campaignRequirements.flatMap((requirement) => criteriaByRequirementId.get(requirement.id) ?? []); | |
| const appliedPolicy = policyByCampaignId.get(campaign.id); | |
| campaign.computedCompletion = computeCompletionSummary( | |
| campaignRequirements.map((requirement) => ({ | |
| id: requirement.id, | |
| criterionIds: requirement.criterionIds, | |
| })), | |
| campaignCriteria.map((criterion) => ({ | |
| id: criterion.id, | |
| evidence: criterion.evidenceIds | |
| .map((evidenceId) => evidence.find((entry) => entry.id === evidenceId)) | |
| .filter((entry): entry is EvidenceNode => Boolean(entry)) | |
| .map((entry) => ({ | |
| id: entry.id, | |
| result: entry.result, | |
| producedAt: entry.producedAt, | |
| })), | |
| })), | |
| { | |
| policy: appliedPolicy | |
| ? { | |
| id: appliedPolicy.id, | |
| coverageThreshold: appliedPolicy.coverageThreshold, | |
| requireAllCriteria: appliedPolicy.requireAllCriteria, | |
| requireEvidence: appliedPolicy.requireEvidence, | |
| } | |
| : undefined, | |
| manualComplete: campaign.status === 'DONE', | |
| }, | |
| ); | |
| for (const campaign of campaigns) { | |
| const manualComplete = campaign.status === 'DONE'; | |
| const memberQuests = questsByCampaignId.get(campaign.id); | |
| if (memberQuests && memberQuests.length > 0) { | |
| campaign.status = deriveCampaignStatusFromQuests(memberQuests); | |
| } | |
| const questIds = new Set((memberQuests ?? []).map((quest) => quest.id)); | |
| const campaignRequirements = requirements.filter((requirement) => requirement.taskIds.some((taskId) => questIds.has(taskId))); | |
| const campaignCriteria = campaignRequirements.flatMap((requirement) => criteriaByRequirementId.get(requirement.id) ?? []); | |
| const appliedPolicy = policyByCampaignId.get(campaign.id); | |
| campaign.computedCompletion = computeCompletionSummary( | |
| campaignRequirements.map((requirement) => ({ | |
| id: requirement.id, | |
| criterionIds: requirement.criterionIds, | |
| })), | |
| campaignCriteria.map((criterion) => ({ | |
| id: criterion.id, | |
| evidence: criterion.evidenceIds | |
| .map((evidenceId) => evidence.find((entry) => entry.id === evidenceId)) | |
| .filter((entry): entry is EvidenceNode => Boolean(entry)) | |
| .map((entry) => ({ | |
| id: entry.id, | |
| result: entry.result, | |
| producedAt: entry.producedAt, | |
| })), | |
| })), | |
| { | |
| policy: appliedPolicy | |
| ? { | |
| id: appliedPolicy.id, | |
| coverageThreshold: appliedPolicy.coverageThreshold, | |
| requireAllCriteria: appliedPolicy.requireAllCriteria, | |
| requireEvidence: appliedPolicy.requireEvidence, | |
| } | |
| : undefined, | |
| manualComplete, | |
| }, | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/infrastructure/GraphContext.ts` around lines 752 - 789, Before calling
deriveCampaignStatusFromQuests and overwriting campaign.status, capture whether
the campaign was manually marked DONE (e.g., const wasManuallyDone =
campaign.status === 'DONE') and then use that captured value when setting the
manualComplete option passed into computeCompletionSummary; update references to
campaign.status only after you’ve stored the manual flag so
computeCompletionSummary receives the original manual state rather than the
derived status. Ensure changes touch the loop where
deriveCampaignStatusFromQuests, campaign.status assignment, and the
computeCompletionSummary call occur.
Comment on lines
+84
to
+86
| q.computedCompletion?.discrepancy | ||
| ? style.styled(style.theme.semantic.warning, '!') | ||
| : (q.assignedTo ?? '—'), |
There was a problem hiding this comment.
Keep the assignee visible when a quest has a completion discrepancy.
This turns the Assigned column into a lone !, so the owner disappears on the exact rows that need follow-up. Surface the warning alongside the assignee, or move the warning into its own column.
One low-churn way to preserve both signals
- q.computedCompletion?.discrepancy
- ? style.styled(style.theme.semantic.warning, '!')
- : (q.assignedTo ?? '—'),
+ q.computedCompletion?.discrepancy
+ ? `${style.styled(style.theme.semantic.warning, '!')} ${q.assignedTo ?? '—'}`
+ : (q.assignedTo ?? '—'),📝 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.
Suggested change
| q.computedCompletion?.discrepancy | |
| ? style.styled(style.theme.semantic.warning, '!') | |
| : (q.assignedTo ?? '—'), | |
| q.computedCompletion?.discrepancy | |
| ? `${style.styled(style.theme.semantic.warning, '!')} ${q.assignedTo ?? '—'}` | |
| : (q.assignedTo ?? '—'), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tui/render-status.ts` around lines 84 - 86, The Assigned cell is replaced
by a lone "!" when q.computedCompletion?.discrepancy is true; instead preserve
the assignee and show the warning alongside it. In render-status.ts where the
tuple element uses q.computedCompletion?.discrepancy ?
style.styled(style.theme.semantic.warning, '!') : (q.assignedTo ?? '—'), change
the branch so that when discrepancy is present you render both the styled
warning and the assignee (e.g., prefix or suffix the styled '!' to (q.assignedTo
?? '—')), or alternatively move the warning into a dedicated column; update the
expression that builds that cell (referencing q.computedCompletion?.discrepancy,
q.assignedTo, and style.styled) so the owner remains visible whenever a
discrepancy exists.
Comment on lines
+57
to
+74
| function makeCtx(): CliContext { | ||
| return { | ||
| agentId: 'agent.hal', | ||
| identity: { agentId: 'agent.hal', source: 'default', origin: null }, | ||
| json: true, | ||
| graphPort: {} as CliContext['graphPort'], | ||
| style: {} as CliContext['style'], | ||
| ok: vi.fn(), | ||
| warn: vi.fn(), | ||
| muted: vi.fn(), | ||
| print: vi.fn(), | ||
| fail: vi.fn((msg: string) => { | ||
| throw new Error(msg); | ||
| }), | ||
| failWithData: vi.fn(), | ||
| jsonOut: vi.fn(), | ||
| } as unknown as CliContext; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and examine the test file
git ls-files | grep -E "AgentCommands\.test\.ts"Repository: flyingrobots/xyph
Length of output: 92
🏁 Script executed:
# Check if file exists and read the relevant lines
if [ -f "test/unit/AgentCommands.test.ts" ]; then
echo "=== File found, reading lines 50-80 ===";
sed -n '50,80p' test/unit/AgentCommands.test.ts;
else
echo "File not found, searching for similar files";
find . -name "*AgentCommands*" -type f 2>/dev/null | head -20;
fiRepository: flyingrobots/xyph
Length of output: 863
🏁 Script executed:
# Get file size to understand its scope
wc -l test/unit/AgentCommands.test.ts 2>/dev/null || echo "File not found"Repository: flyingrobots/xyph
Length of output: 96
🏁 Script executed:
# Search for CliContext type definition
rg "type CliContext|interface CliContext" -A 20 --max-count=1Repository: flyingrobots/xyph
Length of output: 1105
Remove the as unknown as CliContext double cast.
The cast hides fixture drift from the type checker, so tests stop telling us when the command layer starts depending on a new CliContext field. Use as never only on individual nested stubs instead.
Safer fixture shape
function makeCtx(): CliContext {
return {
agentId: 'agent.hal',
identity: { agentId: 'agent.hal', source: 'default', origin: null },
json: true,
- graphPort: {} as CliContext['graphPort'],
- style: {} as CliContext['style'],
+ graphPort: {} as never,
+ style: {} as never,
ok: vi.fn(),
warn: vi.fn(),
muted: vi.fn(),
print: vi.fn(),
@@
failWithData: vi.fn(),
jsonOut: vi.fn(),
- } as unknown as CliContext;
+ };
}📝 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.
Suggested change
| function makeCtx(): CliContext { | |
| return { | |
| agentId: 'agent.hal', | |
| identity: { agentId: 'agent.hal', source: 'default', origin: null }, | |
| json: true, | |
| graphPort: {} as CliContext['graphPort'], | |
| style: {} as CliContext['style'], | |
| ok: vi.fn(), | |
| warn: vi.fn(), | |
| muted: vi.fn(), | |
| print: vi.fn(), | |
| fail: vi.fn((msg: string) => { | |
| throw new Error(msg); | |
| }), | |
| failWithData: vi.fn(), | |
| jsonOut: vi.fn(), | |
| } as unknown as CliContext; | |
| } | |
| function makeCtx(): CliContext { | |
| return { | |
| agentId: 'agent.hal', | |
| identity: { agentId: 'agent.hal', source: 'default', origin: null }, | |
| json: true, | |
| graphPort: {} as never, | |
| style: {} as never, | |
| ok: vi.fn(), | |
| warn: vi.fn(), | |
| muted: vi.fn(), | |
| print: vi.fn(), | |
| fail: vi.fn((msg: string) => { | |
| throw new Error(msg); | |
| }), | |
| failWithData: vi.fn(), | |
| jsonOut: vi.fn(), | |
| }; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/unit/AgentCommands.test.ts` around lines 57 - 74, The test fixture
makeCtx currently suppresses type errors by returning the object with "as
unknown as CliContext"; remove that double cast and make the function return a
proper CliContext shape instead — keep the function named makeCtx and the inner
stubbed properties (ok, warn, muted, print, fail, failWithData, jsonOut,
graphPort, style, identity) but type the return as CliContext and replace any
broad casts with targeted "as never" on individual nested stubs that genuinely
cannot be fully typed; ensure fail keeps its implementation signature (fail:
vi.fn((msg: string) => { throw new Error(msg); })) and adjust any missing fields
so the compiler surfaces real drift rather than being silenced by the
unknown->CliContext cast.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/domain/services/AgentContextService.ts (1)
96-115:⚠️ Potential issue | 🟠 MajorBuild the context from one graph materialization.
fetchSnapshot()andfetchEntityDetail()can observe different frontier states, so the returneddetail.questDetailcan disagree with the dependency and recommendation data computed fromsnapshot. That makes a singlefetch()result internally inconsistent; please derive both from the same materialized snapshot.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/domain/services/AgentContextService.ts` around lines 96 - 115, The fetch method currently calls graphCtx.fetchSnapshot() and graphCtx.fetchEntityDetail(id) separately which can observe different frontier states and produce inconsistent data; change it to materialize a single snapshot and derive both the entity detail and dependency/recommendations from that same snapshot: call graphCtx.fetchSnapshot() once, then use that materialized snapshot to obtain the entity detail (e.g., a graphCtx.fetchEntityDetailFromSnapshot(snapshot, id) or equivalent API) before computing readiness, dependency via buildAgentDependencyContext(snapshot, quest), and recommended actions so all values come from the same snapshot.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cli/commands/agent.ts`:
- Around line 604-610: The handoff command currently only treats outcome.result
=== 'rejected' as an error; update the conditional that checks outcome.result in
the handoff handler to treat both 'rejected' and 'partial-failure' the same way
(mirror the act command logic). When outcome.result is 'rejected' or
'partial-failure', derive the reason from outcome.validation.reasons[0] or
fallback to `Action 'handoff' was rejected/partially failed`, and return using
the same branches: ctx.failWithData(reason, { ...outcome }) when ctx.json is
truthy and ctx.fail(`[REJECTED] ${reason}`) (or a matching message) otherwise.
Ensure this change references the existing outcome.result,
outcome.validation.reasons, ctx.failWithData, ctx.fail and does not alter
executeHandoff behavior beyond error handling.
In `@src/cli/commands/submission.ts`:
- Around line 210-223: The current merge gate uses
graphCtx.fetchEntityDetail(questId) which returns quest-scoped detail and can
point to a different submission than the one being merged; update the logic to
load or validate the submission-specific detail for submissionId (e.g. call a
submission-scoped fetch or locate the submission within detail.questDetail and
ensure its id === submissionId) before calling assessSettlementGate; pass that
submission-scoped detail into assessSettlementGate (instead of
detail?.questDetail) and when returning failure ensure the payload uses the
validated submissionId (not detail.questDetail.submission.id) in
formatSettlementGateFailure and settlementGateFailureData.
In `@test/unit/AgentActionService.test.ts`:
- Around line 273-275: The test mock for GuildSealService.sign is missing fields
required by the GuildSeal type, causing guildSeal.sig to be undefined when
AgentActionService writes the seal; update the mock (the function mocked as
GuildSealService.sign in the test) to return a complete GuildSeal object
including sig, payloadDigest, and sealedAt (e.g., provide a string sig, the
payloadDigest value expected by mocks.payloadDigest, and a sealedAt timestamp)
so that AgentActionService's access to guildSeal.sig succeeds.
---
Duplicate comments:
In `@src/domain/services/AgentContextService.ts`:
- Around line 96-115: The fetch method currently calls graphCtx.fetchSnapshot()
and graphCtx.fetchEntityDetail(id) separately which can observe different
frontier states and produce inconsistent data; change it to materialize a single
snapshot and derive both the entity detail and dependency/recommendations from
that same snapshot: call graphCtx.fetchSnapshot() once, then use that
materialized snapshot to obtain the entity detail (e.g., a
graphCtx.fetchEntityDetailFromSnapshot(snapshot, id) or equivalent API) before
computing readiness, dependency via buildAgentDependencyContext(snapshot,
quest), and recommended actions so all values come from the same snapshot.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1bb95b37-5bbf-4963-8b56-2fd881ee4676
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (15)
CHANGELOG.mddocs/canonical/AGENT_PROTOCOL.mdpackage.jsonsrc/cli/commands/agent.tssrc/cli/commands/coordination.tssrc/cli/commands/submission.tssrc/domain/services/AgentActionService.tssrc/domain/services/AgentBriefingService.tssrc/domain/services/AgentContextService.tssrc/domain/services/AgentRecommender.tssrc/infrastructure/adapters/WarpSubmissionAdapter.tstest/unit/AgentActionService.test.tstest/unit/AgentCommands.test.tstest/unit/AgentContextService.test.tstest/unit/SignedSettlementCommands.test.ts
✅ Files skipped from review due to trivial changes (1)
- package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/canonical/AGENT_PROTOCOL.md
Comment on lines
+604
to
+610
| if (outcome.result === 'rejected') { | ||
| const reason = outcome.validation.reasons[0] ?? `Action 'handoff' was rejected`; | ||
| if (ctx.json) { | ||
| return ctx.failWithData(reason, { ...outcome }); | ||
| } | ||
| return ctx.fail(`[REJECTED] ${reason}`); | ||
| } |
There was a problem hiding this comment.
Handoff command does not handle partial-failure result.
The act command at Lines 554-570 correctly checks for both 'rejected' and 'partial-failure' outcomes. However, the handoff command only checks for 'rejected'. While executeHandoff currently doesn't produce partial failures, this inconsistency could silently succeed when a partial failure occurs if the implementation changes.
Align error handling with the act command
- if (outcome.result === 'rejected') {
+ if (outcome.result === 'rejected' || outcome.result === 'partial-failure') {
- const reason = outcome.validation.reasons[0] ?? `Action 'handoff' was rejected`;
+ const reason = outcome.result === 'partial-failure'
+ ? String(
+ (outcome.details?.['partialFailure'] as { message?: unknown } | undefined)?.message
+ ?? outcome.validation.reasons[0]
+ ?? `Action 'handoff' completed with a partial failure`,
+ )
+ : outcome.validation.reasons[0] ?? `Action 'handoff' was rejected`;
if (ctx.json) {
return ctx.failWithData(reason, { ...outcome });
}
+ if (outcome.result === 'partial-failure') {
+ return ctx.fail(`[PARTIAL FAILURE] ${reason}`);
+ }
return ctx.fail(`[REJECTED] ${reason}`);
}📝 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.
Suggested change
| if (outcome.result === 'rejected') { | |
| const reason = outcome.validation.reasons[0] ?? `Action 'handoff' was rejected`; | |
| if (ctx.json) { | |
| return ctx.failWithData(reason, { ...outcome }); | |
| } | |
| return ctx.fail(`[REJECTED] ${reason}`); | |
| } | |
| if (outcome.result === 'rejected' || outcome.result === 'partial-failure') { | |
| const reason = outcome.result === 'partial-failure' | |
| ? String( | |
| (outcome.details?.['partialFailure'] as { message?: unknown } | undefined)?.message | |
| ?? outcome.validation.reasons[0] | |
| ?? `Action 'handoff' completed with a partial failure`, | |
| ) | |
| : outcome.validation.reasons[0] ?? `Action 'handoff' was rejected`; | |
| if (ctx.json) { | |
| return ctx.failWithData(reason, { ...outcome }); | |
| } | |
| if (outcome.result === 'partial-failure') { | |
| return ctx.fail(`[PARTIAL FAILURE] ${reason}`); | |
| } | |
| return ctx.fail(`[REJECTED] ${reason}`); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli/commands/agent.ts` around lines 604 - 610, The handoff command
currently only treats outcome.result === 'rejected' as an error; update the
conditional that checks outcome.result in the handoff handler to treat both
'rejected' and 'partial-failure' the same way (mirror the act command logic).
When outcome.result is 'rejected' or 'partial-failure', derive the reason from
outcome.validation.reasons[0] or fallback to `Action 'handoff' was
rejected/partially failed`, and return using the same branches:
ctx.failWithData(reason, { ...outcome }) when ctx.json is truthy and
ctx.fail(`[REJECTED] ${reason}`) (or a matching message) otherwise. Ensure this
change references the existing outcome.result, outcome.validation.reasons,
ctx.failWithData, ctx.fail and does not alter executeHandoff behavior beyond
error handling.
Comment on lines
+210
to
+223
| if (shouldAutoSeal && questId) { | ||
| const graphCtx = createGraphContext(ctx.graphPort); | ||
| const detail = await graphCtx.fetchEntityDetail(questId); | ||
| const assessment = assessSettlementGate(detail?.questDetail, 'merge'); | ||
| if (!assessment.allowed) { | ||
| return ctx.failWithData( | ||
| formatSettlementGateFailure(assessment), | ||
| { | ||
| submissionId, | ||
| ...settlementGateFailureData(assessment), | ||
| }, | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Assess merge governance against the requested submission.
fetchEntityDetail(questId) is quest-scoped, so detail.questDetail?.submission can point at a different submission than submissionId when a quest has more than one submission. In that case this gate can allow/block the wrong merge, and the failure payload can even report the other submission’s ID.
⚠️ Minimal guard to stop cross-submission gating
if (shouldAutoSeal && questId) {
const graphCtx = createGraphContext(ctx.graphPort);
const detail = await graphCtx.fetchEntityDetail(questId);
+ const resolvedSubmissionId = detail?.questDetail?.submission?.id;
+ if (resolvedSubmissionId && resolvedSubmissionId !== submissionId) {
+ return ctx.failWithData(
+ `settlement gate resolved ${resolvedSubmissionId} for ${questId}, expected ${submissionId}`,
+ {
+ questId,
+ submissionId,
+ resolvedSubmissionId,
+ code: 'submission-detail-mismatch',
+ },
+ );
+ }
const assessment = assessSettlementGate(detail?.questDetail, 'merge');Longer term, the gate should consume detail keyed by the validated submissionId, not the quest’s current summary.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli/commands/submission.ts` around lines 210 - 223, The current merge
gate uses graphCtx.fetchEntityDetail(questId) which returns quest-scoped detail
and can point to a different submission than the one being merged; update the
logic to load or validate the submission-specific detail for submissionId (e.g.
call a submission-scoped fetch or locate the submission within
detail.questDetail and ensure its id === submissionId) before calling
assessSettlementGate; pass that submission-scoped detail into
assessSettlementGate (instead of detail?.questDetail) and when returning failure
ensure the payload uses the validated submissionId (not
detail.questDetail.submission.id) in formatSettlementGateFailure and
settlementGateFailureData.
Comment on lines
+273
to
+275
| mocks.hasPrivateKey.mockReturnValue(true); | ||
| mocks.sign.mockResolvedValue({ keyId: 'did:key:test', alg: 'ed25519' }); | ||
| mocks.payloadDigest.mockReturnValue('blake3:test'); |
There was a problem hiding this comment.
Mock for GuildSealService.sign is missing required fields.
Per GuildSealService.sign() (Context snippet 1), the return type GuildSeal includes sig, payloadDigest, and sealedAt. The production code at Lines 1652-1654 in AgentActionService.ts accesses guildSeal.sig when writing the seal. The current mock only returns keyId and alg, so guildSeal.sig will be undefined.
Complete the mock return value
- mocks.sign.mockResolvedValue({ keyId: 'did:key:test', alg: 'ed25519' });
+ mocks.sign.mockResolvedValue({
+ keyId: 'did:key:test',
+ alg: 'ed25519',
+ sig: 'test-signature-hex',
+ payloadDigest: 'blake3:test-digest',
+ sealedAt: Date.now(),
+ });📝 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.
Suggested change
| mocks.hasPrivateKey.mockReturnValue(true); | |
| mocks.sign.mockResolvedValue({ keyId: 'did:key:test', alg: 'ed25519' }); | |
| mocks.payloadDigest.mockReturnValue('blake3:test'); | |
| mocks.hasPrivateKey.mockReturnValue(true); | |
| mocks.sign.mockResolvedValue({ | |
| keyId: 'did:key:test', | |
| alg: 'ed25519', | |
| sig: 'test-signature-hex', | |
| payloadDigest: 'blake3:test-digest', | |
| sealedAt: Date.now(), | |
| }); | |
| mocks.payloadDigest.mockReturnValue('blake3:test'); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/unit/AgentActionService.test.ts` around lines 273 - 275, The test mock
for GuildSealService.sign is missing fields required by the GuildSeal type,
causing guildSeal.sig to be undefined when AgentActionService writes the seal;
update the mock (the function mocked as GuildSealService.sign in the test) to
return a complete GuildSeal object including sig, payloadDigest, and sealedAt
(e.g., provide a string sig, the payloadDigest value expected by
mocks.payloadDigest, and a sealedAt timestamp) so that AgentActionService's
access to guildSeal.sig succeeds.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/cli/commands/doctor.ts (1)
61-74: Consider adding a type-safe serialization helper to avoidas unknown ascasts.The casts at lines 65 and 71 work but bypass type safety. A cleaner approach would be to add a serialization method or utility:
♻️ Suggested refactor
// In DoctorService.ts or a separate utility export function serializeReport(report: DoctorReport): Record<string, unknown> { return JSON.parse(JSON.stringify(report)); }Then in the command:
- report as unknown as Record<string, unknown>, + serializeReport(report),This is a minor style improvement—the current approach functions correctly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/commands/doctor.ts` around lines 61 - 74, Add a small type-safe serializer for DoctorReport (e.g., export function serializeReport(report: DoctorReport): Record<string, unknown>) and use it in the doctor command instead of the unsafe casts; replace the two occurrences of "report as unknown as Record<string, unknown>" in the ctx.failWithData and ctx.jsonOut calls with serializeReport(report) so the code uses a properly typed conversion (place the helper in DoctorService.ts or a shared utils file and import it into the command).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/cli/commands/doctor.ts`:
- Around line 61-74: Add a small type-safe serializer for DoctorReport (e.g.,
export function serializeReport(report: DoctorReport): Record<string, unknown>)
and use it in the doctor command instead of the unsafe casts; replace the two
occurrences of "report as unknown as Record<string, unknown>" in the
ctx.failWithData and ctx.jsonOut calls with serializeReport(report) so the code
uses a properly typed conversion (place the helper in DoctorService.ts or a
shared utils file and import it into the command).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 502cc2a4-d55d-44f2-b3b7-e59a286596ce
📒 Files selected for processing (6)
CHANGELOG.mdsrc/cli/commands/doctor.tssrc/domain/services/DoctorService.tstest/unit/DoctorCommands.test.tstest/unit/DoctorService.test.tsxyph-actuator.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- CHANGELOG.md
- xyph-actuator.ts
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
test/unit/AgentCommands.test.ts (1)
57-74:⚠️ Potential issue | 🟠 MajorRemove the
as unknown as CliContextfixture cast.This masks interface drift in tests and defeats type-checking where it matters most (the command/context contract). Use targeted
as neveronly on the nested stub fields that cannot be fully modeled, and return the object directly.♻️ Proposed fixture tightening
function makeCtx(): CliContext { return { @@ - graphPort: {} as CliContext['graphPort'], - style: {} as CliContext['style'], + graphPort: {} as never, + style: {} as never, @@ - failWithData: vi.fn(), + failWithData: vi.fn((msg: string) => { + throw new Error(msg); + }), jsonOut: vi.fn(), - } as unknown as CliContext; + }; }Based on learnings,
as neveris accepted in test fixtures, whileas unknown asshould be avoided; and as per coding guidelines,**/*.{ts,tsx}must not cast tounknownto silence the compiler.#!/bin/bash # Verify there are no double-casts suppressing TS checks in this test file. fd 'AgentCommands\.test\.ts$' --exec rg -nP '\bas\s+unknown\s+as\b' {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/AgentCommands.test.ts` around lines 57 - 74, The test fixture currently nukes type-checking by returning makeCtx() with a double-cast "as unknown as CliContext"; remove that final double-cast and return the object directly as a CliContext, tightening types on the stubbed fields instead (use targeted "as never" only for specific properties that cannot be fully modeled, e.g., ok, warn, muted, print, failWithData, jsonOut if necessary) so the makeCtx function preserves the CliContext contract and surfaces interface drift; update the return signature of makeCtx to be CliContext without using "as unknown as CliContext" and cast only the minimal nested fields to never where required.src/domain/services/AgentBriefingService.ts (1)
221-227:⚠️ Potential issue | 🟠 MajorRank executable candidates before source affinity.
Line 222 still sorts by
sourcePrioritybeforeallowed, so blocked assignment/submission items can outrank the only executable frontier/planning action. With small limits,next()can hide the action the agent can actually run.Put actionable work first
candidates.sort((a, b) => - sourcePriority(a.source) - sourcePriority(b.source) || Number(b.allowed) - Number(a.allowed) || + sourcePriority(a.source) - sourcePriority(b.source) || kindPriority(a.kind) - kindPriority(b.kind) || b.confidence - a.confidence || a.targetId.localeCompare(b.targetId) );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/domain/services/AgentBriefingService.ts` around lines 221 - 227, The sort comparator currently orders by sourcePriority before allowed, which lets non-executable items outrank actionable ones; update the comparator in the candidates.sort call so that Number(b.allowed) - Number(a.allowed) is the first comparison (i.e., put allowed/executable status before sourcePriority), then follow with sourcePriority(a.source) - sourcePriority(b.source), then kindPriority, confidence, and targetId to ensure actionable candidates are ranked first; modify the comparator expression around the existing references to sourcePriority, allowed, kindPriority, confidence, and targetId accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cli/commands/show.ts`:
- Line 181: The diagnostics derived from the readiness object are being rendered
twice: once inline via the readiness unmet messages and again when you call
renderDiagnosticsLines(diagnostics) and pass diagnostics into renderQuestDetail;
update the logic around diagnostics creation (the variable diagnostics) so it
excludes readiness-derived entries when readiness.unmet or similar inline
messages are present, or else stop passing diagnostics into renderQuestDetail
for not-ready quests; specifically adjust the code paths that call
renderDiagnosticsLines(diagnostics) and renderQuestDetail(...) to filter out
readiness diagnostics (referencing readiness, diagnostics,
renderDiagnosticsLines, and renderQuestDetail) so readiness failures are shown
only once.
In `@src/domain/services/AgentBriefingService.ts`:
- Around line 439-445: The "no-active-work" alert is being emitted without
checking for pending reviews; update the condition that currently checks
assignments and frontier to also check reviewQueue so the alert is only pushed
when assignments.length === 0 && frontier.length === 0 && reviewQueue.length ===
0. Locate the block in AgentBriefingService where variables assignments,
frontier, alerts and the alert object with code 'no-active-work' are used and
add reviewQueue to the gate that suppresses the alert when review work exists.
In `@src/domain/services/DoctorService.ts`:
- Around line 111-114: The unbounded Promise.all over ids (currently:
Promise.all(ids.map(async (id) => { const raw = await graph.neighbors(id,
direction); return [id, toNeighborEntries(raw)] as const; }))) can overwhelm
graph I/O; change it to a bounded-concurrency approach (e.g., use a concurrency
limiter like p-limit or process ids in fixed-size batches) so only N
graph.neighbors calls run concurrently. Implement this in the same scope where
results is computed (preserving the returned tuple shape [id,
toNeighborEntries(raw)]) and collect the tuples into the same results array
before continuing; ensure error handling and awaiting completion remain the same
so downstream code using results behaves unchanged.
---
Duplicate comments:
In `@src/domain/services/AgentBriefingService.ts`:
- Around line 221-227: The sort comparator currently orders by sourcePriority
before allowed, which lets non-executable items outrank actionable ones; update
the comparator in the candidates.sort call so that Number(b.allowed) -
Number(a.allowed) is the first comparison (i.e., put allowed/executable status
before sourcePriority), then follow with sourcePriority(a.source) -
sourcePriority(b.source), then kindPriority, confidence, and targetId to ensure
actionable candidates are ranked first; modify the comparator expression around
the existing references to sourcePriority, allowed, kindPriority, confidence,
and targetId accordingly.
In `@test/unit/AgentCommands.test.ts`:
- Around line 57-74: The test fixture currently nukes type-checking by returning
makeCtx() with a double-cast "as unknown as CliContext"; remove that final
double-cast and return the object directly as a CliContext, tightening types on
the stubbed fields instead (use targeted "as never" only for specific properties
that cannot be fully modeled, e.g., ok, warn, muted, print, failWithData,
jsonOut if necessary) so the makeCtx function preserves the CliContext contract
and surfaces interface drift; update the return signature of makeCtx to be
CliContext without using "as unknown as CliContext" and cast only the minimal
nested fields to never where required.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 41c76d1e-7fc1-4b82-9438-1a0854e11081
📒 Files selected for processing (17)
src/cli/commands/agent.tssrc/cli/commands/doctor.tssrc/cli/commands/show.tssrc/cli/context.tssrc/cli/renderDiagnostics.tssrc/domain/models/diagnostics.tssrc/domain/services/AgentBriefingService.tssrc/domain/services/AgentContextService.tssrc/domain/services/DiagnosticService.tssrc/domain/services/DoctorService.tstest/unit/AgentBriefingService.test.tstest/unit/AgentCommands.test.tstest/unit/CliJsonOutput.test.tstest/unit/DiagnosticService.test.tstest/unit/DoctorCommands.test.tstest/unit/DoctorService.test.tstest/unit/ShowCommands.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/cli/commands/agent.ts
- test/unit/DoctorCommands.test.ts
- src/cli/commands/doctor.ts
|
|
||
| lines.push(...renderNarrativeLines('Documents', detail.documents)); | ||
| lines.push(...renderNarrativeLines('Comments', detail.comments)); | ||
| lines.push(...renderDiagnosticsLines(diagnostics)); |
There was a problem hiding this comment.
Avoid rendering readiness failures twice.
Line 247 derives diagnostics from the same readiness object whose unmet messages are already rendered inline, and Lines 270-272 pass both into renderQuestDetail(). For not-ready quests, the human show output will repeat the same contract gap.
Also applies to: 247-247, 270-272
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli/commands/show.ts` at line 181, The diagnostics derived from the
readiness object are being rendered twice: once inline via the readiness unmet
messages and again when you call renderDiagnosticsLines(diagnostics) and pass
diagnostics into renderQuestDetail; update the logic around diagnostics creation
(the variable diagnostics) so it excludes readiness-derived entries when
readiness.unmet or similar inline messages are present, or else stop passing
diagnostics into renderQuestDetail for not-ready quests; specifically adjust the
code paths that call renderDiagnosticsLines(diagnostics) and
renderQuestDetail(...) to filter out readiness diagnostics (referencing
readiness, diagnostics, renderDiagnosticsLines, and renderQuestDetail) so
readiness failures are shown only once.
Comment on lines
+439
to
+445
| if (assignments.length === 0 && frontier.length === 0) { | ||
| alerts.push({ | ||
| code: 'no-active-work', | ||
| severity: 'info', | ||
| message: 'No active assignments or READY frontier quests were found.', | ||
| relatedIds: [], | ||
| }); |
There was a problem hiding this comment.
Don't emit no-active-work when reviews are pending.
Line 439 ignores reviewQueue, so a briefing can tell the agent there is no active work while also raising a review-queue alert.
Suppress the alert when review work exists
- if (assignments.length === 0 && frontier.length === 0) {
+ if (assignments.length === 0 && frontier.length === 0 && reviewQueue.length === 0) {
alerts.push({
code: 'no-active-work',
severity: 'info',
message: 'No active assignments or READY frontier quests were found.',
relatedIds: [],
});
}📝 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.
Suggested change
| if (assignments.length === 0 && frontier.length === 0) { | |
| alerts.push({ | |
| code: 'no-active-work', | |
| severity: 'info', | |
| message: 'No active assignments or READY frontier quests were found.', | |
| relatedIds: [], | |
| }); | |
| if (assignments.length === 0 && frontier.length === 0 && reviewQueue.length === 0) { | |
| alerts.push({ | |
| code: 'no-active-work', | |
| severity: 'info', | |
| message: 'No active assignments or READY frontier quests were found.', | |
| relatedIds: [], | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/domain/services/AgentBriefingService.ts` around lines 439 - 445, The
"no-active-work" alert is being emitted without checking for pending reviews;
update the condition that currently checks assignments and frontier to also
check reviewQueue so the alert is only pushed when assignments.length === 0 &&
frontier.length === 0 && reviewQueue.length === 0. Locate the block in
AgentBriefingService where variables assignments, frontier, alerts and the alert
object with code 'no-active-work' are used and add reviewQueue to the gate that
suppresses the alert when review work exists.
Comment on lines
+111
to
+114
| const results = await Promise.all(ids.map(async (id) => { | ||
| const raw = await graph.neighbors(id, direction); | ||
| return [id, toNeighborEntries(raw)] as const; | ||
| })); |
There was a problem hiding this comment.
Bound graph.neighbors fan-out to avoid overload on large graphs.
Promise.all over all node IDs is unbounded and can saturate graph I/O/memory under larger datasets.
⚙️ Proposed bounded batching
async function batchNeighbors(
graph: WarpGraph,
ids: string[],
direction: 'outgoing' | 'incoming' = 'outgoing',
): Promise<Map<string, NeighborEntry[]>> {
const map = new Map<string, NeighborEntry[]>();
- const results = await Promise.all(ids.map(async (id) => {
- const raw = await graph.neighbors(id, direction);
- return [id, toNeighborEntries(raw)] as const;
- }));
-
- for (const [id, neighbors] of results) {
- map.set(id, neighbors);
+ const BATCH_SIZE = 64;
+ for (let i = 0; i < ids.length; i += BATCH_SIZE) {
+ const chunk = ids.slice(i, i + BATCH_SIZE);
+ const results = await Promise.all(chunk.map(async (id) => {
+ const raw = await graph.neighbors(id, direction);
+ return [id, toNeighborEntries(raw)] as const;
+ }));
+ for (const [id, neighbors] of results) {
+ map.set(id, neighbors);
+ }
}
return map;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/domain/services/DoctorService.ts` around lines 111 - 114, The unbounded
Promise.all over ids (currently: Promise.all(ids.map(async (id) => { const raw =
await graph.neighbors(id, direction); return [id, toNeighborEntries(raw)] as
const; }))) can overwhelm graph I/O; change it to a bounded-concurrency approach
(e.g., use a concurrency limiter like p-limit or process ids in fixed-size
batches) so only N graph.neighbors calls run concurrently. Implement this in the
same scope where results is computed (preserving the returned tuple shape [id,
toNeighborEntries(raw)]) and collect the tuples into the same results array
before continuing; ensure error handling and awaiting completion remain the same
so downstream code using results behaves unchanged.
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
briefing,next,context,submissions,act,handoff)Verification
Notes
Summary by CodeRabbit