feat(software-factory): strong validation, runaway-loop guard, retry-with-feedback, queryable status #73

Merged

adamhjk merged 1 commit from software-factory-validation into main 2026-06-24 20:59:19 +00:00

Conversation 2 Commits 1 Files changed 25 +2230 -131

adamhjk commented 2026-06-24 20:42:11 +00:00

Owner

Copy link

Closes swamp-club lab #757 and #786, and removes the STATUS_JSON log-scrape from the driver loop.

Strong validation

Every datum that crosses a stage boundary now carries a declared schema validated at write time: artifacts/evidence must declare a schema (graph-enforced, agent-actionable errors); object payloads compile to strict zod by default (additionalProperties opts out); a stage's resultEvidence gets a built-in {status,runId,outputs?} contract; method/workflow stages can declare an inputsSchema, surfaced as invalidInputs in status.

Runaway-loop guard (#757)

record_dispatch records each stage execution into a cycle-scoped counter; a recorded dispatch is now a precondition for advancing out of a work-bearing stage (work can't be silently skipped); maxDispatchesPerCycle (default 2) hard-fails the third dispatch of an entry with runaway-loop-suspected diagnostics — the within-entry counterpart to maxCycles.

Retry-with-feedback (#786)

record_artifact/record_evidence persist each rejection as a bindable validation-<target> record (write-then-throw, durable) holding the rejected value and path-bearing errors, auto-cleared on a clean record and surfaced in status. A retry binds it back via data.latest(self.name, "validation-<target>"). Runnable as examples/retry-feedback.yaml.

Queryable status

status no longer smuggles its self-describing packet out as a STATUS_JSON log line for the driver to scrape and re-parse — an out-of-band, stringly-typed channel that floods the interpreter's context with a maximal blob every loop. It now persists the view as a status-<workItem> data record (and status-_factory for the fleet overview) and returns it as a data handle, so the driver pulls exactly the fields it needs with swamp data query --select (satisfied transitions, failing-gate reasons, the resolved work spec). The new status resource is a read-time materialized view, garbage-collected to the last few refreshes. A test evaluates every --select projection shown in the skill docs through the platform's own cel-js query path, so the documented queries can't drift from what runs.

Verification

164 tests pass; deno check/lint/fmt clean; lockfile frozen. DESIGN, README, manifest, and the skill (SKILL/authoring/driving) updated; version bumped to 2026.06.24.1.

🤖 Generated with Claude Code

Closes swamp-club lab #757 and #786, and removes the `STATUS_JSON` log-scrape from the driver loop. ## Strong validation Every datum that crosses a stage boundary now carries a declared schema validated at write time: artifacts/evidence must declare a schema (graph-enforced, agent-actionable errors); object payloads compile to strict zod by default (`additionalProperties` opts out); a stage's `resultEvidence` gets a built-in `{status,runId,outputs?}` contract; method/workflow stages can declare an `inputsSchema`, surfaced as `invalidInputs` in status. ## Runaway-loop guard (#757) `record_dispatch` records each stage execution into a cycle-scoped counter; a recorded dispatch is now a precondition for advancing out of a work-bearing stage (work can't be silently skipped); `maxDispatchesPerCycle` (default 2) hard-fails the third dispatch of an entry with `runaway-loop-suspected` diagnostics — the within-entry counterpart to `maxCycles`. ## Retry-with-feedback (#786) `record_artifact`/`record_evidence` persist each rejection as a bindable `validation-<target>` record (write-then-throw, durable) holding the rejected value and path-bearing errors, auto-cleared on a clean record and surfaced in status. A retry binds it back via `data.latest(self.name, "validation-<target>")`. Runnable as `examples/retry-feedback.yaml`. ## Queryable status `status` no longer smuggles its self-describing packet out as a `STATUS_JSON` log line for the driver to scrape and re-parse — an out-of-band, stringly-typed channel that floods the interpreter's context with a maximal blob every loop. It now persists the view as a `status-<workItem>` data record (and `status-_factory` for the fleet overview) and returns it as a data handle, so the driver pulls exactly the fields it needs with `swamp data query --select` (satisfied transitions, failing-gate reasons, the resolved work spec). The new `status` resource is a read-time materialized view, garbage-collected to the last few refreshes. A test evaluates **every** `--select` projection shown in the skill docs through the platform's own cel-js query path, so the documented queries can't drift from what runs. ## Verification 164 tests pass; `deno check`/`lint`/`fmt` clean; lockfile frozen. DESIGN, README, manifest, and the skill (SKILL/authoring/driving) updated; version bumped to 2026.06.24.1. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

adamhjk added 1 commit 2026-06-24 20:42:11 +00:00

feat(software-factory): strong validation, runaway-loop guard, retry-with-feedback, queryable status ... f80da7c947

Make every datum that crosses a stage boundary carry a declared schema validated at write time, and close the two failure modes from swamp-club lab #757 and #786.

Strong schemas: artifacts and evidence must declare a schema (graph-enforced, hard error with an agent-actionable message); object payloads compile to strict zod by default with additionalProperties as the opt-out; a stage's resultEvidence gets a built-in {status,runId,outputs?} outcome schema; method/workflow stages can declare inputsSchema, surfaced as invalidInputs in status; resource envelopes are typed zod.

Runaway-loop guard (#757): record_dispatch records each stage execution into a cycle-scoped counter; the stage-executed check makes a recorded dispatch a precondition for advancing out of a work-bearing stage (work can't be skipped); maxDispatchesPerCycle (default 2) hard-fails the third dispatch of an entry with runaway-loop-suspected diagnostics. The within-entry counterpart to maxCycles.

Retry-with-feedback (#786): record_artifact/record_evidence record each rejection as a bindable validation-<target> record (write-then-throw, confirmed durable by the platform's method-execution layer) holding the rejected value and path-bearing errors, auto-cleared on a clean record and surfaced in status. A retry binds it back into the model via data.latest(self.name, "validation-<target>"). Bundled runnable as examples/retry-feedback.yaml.

Queryable status: status no longer smuggles its self-describing packet out as a STATUS_JSON log line for the driver to scrape and re-parse — an out-of-band, stringly-typed channel that floods the interpreter's context with a maximal blob every loop. It now persists the view as a status-<workItem> data record (and status-_factory for the fleet overview) and returns it as a data handle, so the driver pulls exactly the fields it needs with `swamp data query --select` (satisfied transitions, failing-gate reasons, the resolved work spec) instead of holding the whole thing in context. New status resource: a read-time materialized view, garbage-collected to the last few refreshes. A test evaluates every --select projection shown in the skill docs through the platform's own cel-js query path, so the documented queries can't drift from what actually runs.

DESIGN, README, manifest, and the skill (SKILL/authoring/driving) updated. 164 tests pass; deno check/lint/fmt clean; lockfile frozen.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

stack72 commented 2026-06-24 20:53:14 +00:00

Owner

Copy link

Code Review

Blocking Issues

None.

Suggestions

1. DESIGN.md item numbering out of order. The three new "resolved" items are numbered 6, 7, 8 but appear before existing item 5 ("CEL evaluation timing") in the file, so the list reads …4, 6, 7, 8, 5… A reader scanning the document will notice the misordering. Consider renumbering 5 → 9 (or reordering the items) so the list is monotonically increasing.

2. advance() test helper silently swallows all record_dispatch errors. The helper wraps record_dispatch in a bare try/catch with no rethrow, so if the call fails for an unexpected reason (e.g. wrong stage, stage.work === undefined) it is silently ignored. For a test helper that is shared across dozens of test cases, a tighter guard (e.g. catch { /* not a work-bearing stage */ } with a narrower check) would prevent future tests from accidentally relying on a silent no-op when they meant to dispatch. This is a test-quality concern only; it does not affect production behaviour.

3. stageExecutionDiagnostics is called twice on a repeat dispatch. Once for the hard-stop guard (attempt > limit, before writing) and once for the warning path (attempt > 1, after writing). Both calls re-evaluate all gate conditions. The function is async and reads the platform data repository, so on a factory with many stages and many gate predicates it could be notably slow for what is essentially a diagnostic log message. A minor optimisation would be to compute diagnostics once and reuse the result. No correctness impact.

## Code Review ### Blocking Issues None. ### Suggestions 1. **DESIGN.md item numbering out of order.** The three new "resolved" items are numbered 6, 7, 8 but appear *before* existing item 5 ("CEL evaluation timing") in the file, so the list reads …4, 6, 7, 8, 5… A reader scanning the document will notice the misordering. Consider renumbering 5 → 9 (or reordering the items) so the list is monotonically increasing. 2. **`advance()` test helper silently swallows all `record_dispatch` errors.** The helper wraps `record_dispatch` in a bare `try/catch` with no rethrow, so if the call fails for an unexpected reason (e.g. wrong stage, `stage.work === undefined`) it is silently ignored. For a test helper that is shared across dozens of test cases, a tighter guard (e.g. `catch { /* not a work-bearing stage */ }` with a narrower check) would prevent future tests from accidentally relying on a silent no-op when they meant to dispatch. This is a test-quality concern only; it does not affect production behaviour. 3. **`stageExecutionDiagnostics` is called twice on a repeat dispatch.** Once for the hard-stop guard (`attempt > limit`, before writing) and once for the warning path (`attempt > 1`, after writing). Both calls re-evaluate all gate conditions. The function is async and reads the platform data repository, so on a factory with many stages and many gate predicates it could be notably slow for what is essentially a diagnostic log message. A minor optimisation would be to compute diagnostics once and reuse the result. No correctness impact.

adamhjk merged commit 11e052b613 into main 2026-06-24 20:59:19 +00:00

adamhjk referenced this pull request from a commit 2026-06-24 20:59:19 +00:00

feat(software-factory): strong validation, runaway-loop guard, retry-with-feedback, queryable status (#73)

adamhjk deleted branch software-factory-validation 2026-06-24 20:59:19 +00:00

stack72 commented 2026-06-24 21:03:32 +00:00

Owner

Copy link

Adversarial Code Review: software-factory extension

Verdict: PASS

No critical or high-severity findings. The code is well-engineered with
defense-in-depth, comprehensive validation, and thorough test coverage.

---

Findings

MEDIUM

M1: Stage/global transition name collision bypasses dispatch requirement

Files: software-factory/extensions/models/software_factory.ts (line 316), software-factory/extensions/models/_lib/definition_schema.ts (isGlobalTransition)

Description:
The function requiresDispatch() determines whether a stage needs agent work
before a transition fires. It calls isGlobalTransition(args, transitionName)
which checks whether any globalTransition has that name. If a stage-level
transition shares its name with a global transition, isGlobalTransition
returns true and requiresDispatch returns false -- the dispatch requirement
for that stage transition is silently bypassed.

The stage-executed check (line 476) has the same issue: it also calls
isGlobalTransition by name, so it would skip the stage-executed check for
a stage transition that happens to share a name with a global one.

validateGraph() does not check for name collisions between stage transitions
and global transitions, so there is no compile-time guard against this.

Impact: A definition author who reuses a global transition name on a
stage transition would find that the stage can be advanced without dispatching
work -- the gate and dispatch guards are bypassed for that transition. This
is a definition-level mistake but the engine silently accepts it instead of
catching it at validation time.

Recommendation: Add a check in validateGraph() that flags when a stage
transition name collides with a global transition name.

---

LOW

L1: No exhaustive default in compileDeclaredSchema switch

File: software-factory/extensions/models/_lib/artifact_schema.ts (lines 100-149)

Description:
The switch on schema.type handles object, string, number, integer, boolean,
and array but has no default case. If a future type is added to the TypeScript
union without updating this switch, the function would silently return
undefined (via fall-through), which would then fail at a less obvious call site.

Impact: Low -- the zod schema for the type field constrains inputs, so
an unhandled type cannot reach this code path today. A future union extension
could introduce a silent bug.

L2: Expression regex edge case with CEL map literals

File: software-factory/extensions/models/_lib/bindings.ts (line 98)

Description:
EXPRESSION_PATTERN uses a non-greedy match that stops at the first closing
double-brace sequence. A CEL expression containing a map literal (which uses
curly braces) would be truncated, producing a parse error.

Impact: Low -- current examples and docs do not use map literals in
binding expressions, and the CEL environment is scoped to data lookups that
would not need them. A future power-user might hit this.

L3: Documentation inconsistency on evidence schemas

File: software-factory/.claude/skills/software-factory/references/driving.md (line 225)

Description:
The driving guide says Evidence is schema-free but the code now requires
evidence to declare schemas (validateGraph enforces this; record_evidence
validates payloads against declared schemas). This contradicts the current
implementation.

Impact: Low -- misleading documentation could cause a definition author
to omit evidence schemas and then get a validation error.

L4: User-supplied pattern passed to RegExp without ReDoS protection

File: software-factory/extensions/models/_lib/artifact_schema.ts (line 106)

Description:
When a declared schema has a pattern field, it is passed directly to
new RegExp() via z.string().regex(). A maliciously crafted pattern with
catastrophic backtracking could cause denial of service during validation.

Impact: Low -- the pattern comes from the factory definition (globalArguments),
which is authored by the same team that operates the factory, not by
untrusted external input. An adversarial definition author already has full
control over the factory behavior.

---

Summary

The software-factory extension implements a well-structured model-driven
state machine with strong defensive properties:

• Validation depth: Zod schemas validate every envelope at write and read
  time; declared schemas compile to zod and reject malformed payloads with
  path-bearing errors that feed back into retries.
• Graph validation: validateGraph catches structural errors (duplicate
  names, unreachable stages, acyclic reviews, missing schemas, invalid gate
  references) before the engine runs.
• Concurrency safety: Each work item is namespaced by slug; era filtering
  after reset prevents stale records from leaking into a new run.
• Runaway-loop guard: Dispatch counting with recorded state (not agent
  memory) prevents infinite dispatch loops.
• Defense in depth: Checks are advisory (skippable), but method bodies
  re-validate gates, and cycle limits have human-approval override paths.
• Test coverage: 1787 lines of tests with an in-memory versioned store,
  real CEL evaluation, and edge-case coverage for cycles, resets, validation
  feedback, and parallel work items.

The one MEDIUM finding (name collision bypass) is a validation gap rather
than a runtime bug -- it requires a definition author to accidentally reuse
a global transition name on a stage transition. Adding a cross-validation
check in validateGraph would close it.

# Adversarial Code Review: software-factory extension ## Verdict: PASS No critical or high-severity findings. The code is well-engineered with defense-in-depth, comprehensive validation, and thorough test coverage. --- ## Findings ### MEDIUM #### M1: Stage/global transition name collision bypasses dispatch requirement **Files:** software-factory/extensions/models/software_factory.ts (line 316), software-factory/extensions/models/_lib/definition_schema.ts (isGlobalTransition) **Description:** The function requiresDispatch() determines whether a stage needs agent work before a transition fires. It calls isGlobalTransition(args, transitionName) which checks whether any globalTransition has that name. If a stage-level transition shares its name with a global transition, isGlobalTransition returns true and requiresDispatch returns false -- the dispatch requirement for that stage transition is silently bypassed. The stage-executed check (line 476) has the same issue: it also calls isGlobalTransition by name, so it would skip the stage-executed check for a stage transition that happens to share a name with a global one. validateGraph() does not check for name collisions between stage transitions and global transitions, so there is no compile-time guard against this. **Impact:** A definition author who reuses a global transition name on a stage transition would find that the stage can be advanced without dispatching work -- the gate and dispatch guards are bypassed for that transition. This is a definition-level mistake but the engine silently accepts it instead of catching it at validation time. **Recommendation:** Add a check in validateGraph() that flags when a stage transition name collides with a global transition name. --- ### LOW #### L1: No exhaustive default in compileDeclaredSchema switch **File:** software-factory/extensions/models/_lib/artifact_schema.ts (lines 100-149) **Description:** The switch on schema.type handles object, string, number, integer, boolean, and array but has no default case. If a future type is added to the TypeScript union without updating this switch, the function would silently return undefined (via fall-through), which would then fail at a less obvious call site. **Impact:** Low -- the zod schema for the type field constrains inputs, so an unhandled type cannot reach this code path today. A future union extension could introduce a silent bug. #### L2: Expression regex edge case with CEL map literals **File:** software-factory/extensions/models/_lib/bindings.ts (line 98) **Description:** EXPRESSION_PATTERN uses a non-greedy match that stops at the first closing double-brace sequence. A CEL expression containing a map literal (which uses curly braces) would be truncated, producing a parse error. **Impact:** Low -- current examples and docs do not use map literals in binding expressions, and the CEL environment is scoped to data lookups that would not need them. A future power-user might hit this. #### L3: Documentation inconsistency on evidence schemas **File:** software-factory/.claude/skills/software-factory/references/driving.md (line 225) **Description:** The driving guide says Evidence is schema-free but the code now requires evidence to declare schemas (validateGraph enforces this; record_evidence validates payloads against declared schemas). This contradicts the current implementation. **Impact:** Low -- misleading documentation could cause a definition author to omit evidence schemas and then get a validation error. #### L4: User-supplied pattern passed to RegExp without ReDoS protection **File:** software-factory/extensions/models/_lib/artifact_schema.ts (line 106) **Description:** When a declared schema has a pattern field, it is passed directly to new RegExp() via z.string().regex(). A maliciously crafted pattern with catastrophic backtracking could cause denial of service during validation. **Impact:** Low -- the pattern comes from the factory definition (globalArguments), which is authored by the same team that operates the factory, not by untrusted external input. An adversarial definition author already has full control over the factory behavior. --- ## Summary The software-factory extension implements a well-structured model-driven state machine with strong defensive properties: - **Validation depth:** Zod schemas validate every envelope at write and read time; declared schemas compile to zod and reject malformed payloads with path-bearing errors that feed back into retries. - **Graph validation:** validateGraph catches structural errors (duplicate names, unreachable stages, acyclic reviews, missing schemas, invalid gate references) before the engine runs. - **Concurrency safety:** Each work item is namespaced by slug; era filtering after reset prevents stale records from leaking into a new run. - **Runaway-loop guard:** Dispatch counting with recorded state (not agent memory) prevents infinite dispatch loops. - **Defense in depth:** Checks are advisory (skippable), but method bodies re-validate gates, and cycle limits have human-approval override paths. - **Test coverage:** 1787 lines of tests with an in-memory versioned store, real CEL evaluation, and edge-case coverage for cycles, resets, validation feedback, and parallel work items. The one MEDIUM finding (name collision bypass) is a validation gap rather than a runtime bug -- it requires a definition author to accidentally reuse a global transition name on a stage transition. Adding a cross-validation check in validateGraph would close it.

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

2 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

swamp-club/swamp-extensions!73

No description provided.