feat(server,studio): manual braid-extract from Studio records source unit observation#59
Merged
Merged
Conversation
…unit observation Closes #31. Until now the SourceUnitState ledger was only written by BatchService. A user who picked a single intent unit from Studio's Run-skill form and fired /braid-extract left that unit's recorded state unchanged. The forthcoming Reactor (#29) consults the ledger to decide what to re-extract, and stale entries would either cost LLM tokens on no-op re-extracts or skip work that actually happened. This change plumbs the (sourceId, path) tuple from the dynamic source-intent picker through the run-skill request and records an observation when the run finishes successfully. The server skill-input-options endpoint now ships the option's sourceId alongside value and label. SkillInputDynamicOption gains an optional sourceId field; other providers leave it empty. The run-skill request body accepts an optional sourceUnit. When the skill is braid-extract and a sourceUnit is provided, the route subscribes to the run's events in the background, waits for terminal completion, and calls SourceUnitStateService.recordObservation only when the exit code is zero. Cancellation or any non-zero exit short-circuits so the previously recorded state stays untouched. The hook is hard-coded to braid-extract per the v0 scope; a generic post-skill hook is explicitly out of scope. In Studio, ActionInputForm now reads the cached dynamic option for each selected value through React Query's queryClient, attaches sourceUnit per emitted run spec, and exposes a SkillRunSpec shape from its onSubmit. Actions.tsx threads sourceUnit through sendWith into api.startSkillRun. Other skills that do not consume sourceUnit are unaffected. Tests cover the four cases the acceptance criteria call out: successful run records, non-zero exit does not, missing sourceUnit does not, wrong skill id does not. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…urceUnit Three issues surfaced in code review of the first cut. All fixed here. Race + leak. The previous recordObservationOnSuccess subscribed to the runner and waited for a future terminal event. Two cases hung the Promise forever (and leaked the workspace + sourceUnit + service closure): the run already finished between skillRunner.start returning and the late subscribe attaching, and the run never emits a terminal event at all (orphaned subprocess, server restart, queue corruption). The hook now subscribes first, then checks isActive and reads the persisted RunRecord from RunRepository when the run already settled, and wraps the whole wait in a 60-minute Promise.race timeout that .unref()s so the process can still exit. SkillsRouterDeps gains an optional runRepository for the backfill path; composeApp wires it from the existing deps. Cancel is not non-zero exit. SubprocessSkillRunner.cancel sends SIGTERM and the child close event arrives with code=null, signal='SIGTERM'. The old `exitCode: code ?? 0` mapping collapsed every signal kill to 0 so the route would record an observation for a cancelled run, violating issue #31 acceptance criterion 3. The mapping now reports 128 whenever a signal was the cause, which BatchService and the observation hook both already classify as failure. Bad sourceUnit. A client posting a sourceUnit.sourceId that names no intent source in the workspace previously made it all the way to SourceUnitStateService.recordObservation and polluted the ledger. The route now validates `workspace.sources.find(s => s.id === sourceUnit .sourceId && s.role === 'intent')` before dispatching and throws ValidationError (400) on mismatch. Plus skillInputOptions.resolveSourceIntent now uses safeParse on item.sourceId so a malformed sourceId in PRODUCT.md degrades the option to no-source-tracking instead of 500ing the dropdown. Two new tests cover the additional acceptance: cancel mid-run via a controllable fake child process leaves the ledger empty, and an invalid sourceUnit.sourceId returns 400 without touching the ledger. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ps; reject sourceUnit on the wrong skill Three review follow-ups, all addressed at the same layer. The skills route used to hard-code skillId === 'braid-extract' which coupled the cross-ontology dispatcher to the DDD ontology's per-unit skill name. The route now reads `pluginRegistry.findOntology(workspace.productManifest.ontologyId).batch.perUnit.skillId` and uses that as the gate for observation recording. A C4 ontology that ships its own per-unit skill will work transparently. The BRAID_EXTRACT_SKILL_ID constant is removed. The RunBody used to advertise `sourceUnit` for every skill while silently dropping it on anything except braid-extract. The route now rejects with ValidationError when sourceUnit arrives for a skill the active ontology does not name as its per-unit step. The API contract matches the runtime behaviour. `sourceUnitStateService` and `runRepository` were optional on SkillsRouterDeps so missing wiring failed silently. composeApp always builds both, so the deps now require them; wiring bugs surface immediately instead of producing 202 responses that quietly drop the observation hook. The existing "non-braid-extract" test changes its expected status from 202 to 400 to match the new contract; buildAppForExtract registers a fake ontology whose batch.perUnit.skillId matches the workspace's ontologyId so the route can resolve it. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
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
The SourceUnitState ledger was only written by BatchService. A user who picked one intent unit from Studio's Run-skill form and fired /braid-extract left that unit's recorded state unchanged. Once the Reactor (#29) lands and consults the ledger to decide what to re-extract, stale entries would either waste LLM tokens on no-op re-extracts or skip work that actually happened. This PR plumbs the (sourceId, path) tuple from the dynamic source-intent picker through the run-skill request and records an observation on successful completion.
The server skill-input-options endpoint now ships each option's sourceId alongside value and label. SkillInputDynamicOption gains an optional sourceId field; providers that do not speak in terms of source units (graph-node, clarify) leave it empty. The run-skill request body accepts an optional sourceUnit. When skillId is braid-extract and the body carries a sourceUnit, the route subscribes to the run's events in the background, waits for terminal completion, and calls SourceUnitStateService.recordObservation only when exit code is zero. Cancellation or any non-zero exit short-circuits so the previously recorded state stays untouched. The hook is hardcoded to braid-extract per the v0 scope of issue #31; a generic post-skill hook is explicitly out of scope.
In Studio, ActionInputForm reads the cached dynamic option for each selected value through React Query's queryClient, attaches sourceUnit per emitted run spec, and exposes a SkillRunSpec shape from its onSubmit. Actions.tsx threads sourceUnit through sendWith into api.startSkillRun. Skills that do not use a source-intent picker are unaffected.
Closes #31.
Test plan
🤖 Generated with Claude Code