kaykayyali/damascus-orchestrator
1
0
Fork 0
Code Issues 3 Pull Requests Actions Packages Projects Releases Wiki Activity

fix(spec): route any non-empty Ambiguities section to awaiting_human #12

Merged
kaykayyali merged 2 commits from fix/spec-refiner-ambiguity-routing into main 2026-06-24 13:09:26 +00:00
Conversation 1 Commits 2 Files Changed 2 +64 -1
kaykayyali commented 2026-06-24 07:49:12 +00:00
Owner
Copy Link

Summary

Fixes the spec-refiner ambiguity-routing drift (gap note: wiki/queries/damascus-orchestrator/spec-refiner-ambiguity-routing-drift-2026-06-24.md).

The spec-refiner guarded the awaiting_human transition on the Ambiguities section ending with a question-mark character. The contract says any non-empty section should trigger the channel. List-style ambiguities (e.g. "- the auth model is unclear because of X") fell through to build and the human never saw the issue.

Change

Single-file, 8 insertions, 1 deletion in src/damascus/phases.py. Replaces the trailing-? regex check with a non-empty-after-strip check on the parsed section.

# Before:
if "## Ambiguities" in text and re.search(r"\?\s*$", _section(text, "Ambiguities")):
# After:
ambiguities_section = _section(text, "Ambiguities")
if "## Ambiguities" in text and ambiguities_section.strip():

Verification

  • Contract test test_refine_spec_routes_non_empty_ambiguities_to_awaiting_human (PR #11): PASS (was RED on broken code).
  • Full contract suite: 19/19 pass.
  • Unit suite: 10/10 pass.

The contract test is the structural counterpart: it source-greps for the buggy regex literal and asserts it's gone, plus the positive checks for awaiting_human routing and open_human_issue side effect.

Why this is gap-finding-discipline compliant

  • Code-level decision in code PR, with the gap note documenting the rationale, options, and the recommended Option A.
  • The fix is the smallest possible shape (1 condition change), no architectural choice, no follow-up work.
  • Companion contract test (PR #11) was the visible-in-CI red that justified opening this PR — without it, the fix is a 1-line change with no spec.

Out of scope

  • The related spec-refiner file-scope/budget_cycles drift is in PR #10 (a contract test only; the runtime fix needs an LLM session per the skill).
  • §7 Metrics/Analyzer still gated on issue #6 BMAD picks.

Self-review

Heartbeat authored, heartbeat cannot self-approve. Kay's call to merge. The fix is small, the contract test backs it, the gap note documents the rationale.

## Summary Fixes the spec-refiner ambiguity-routing drift (gap note: `wiki/queries/damascus-orchestrator/spec-refiner-ambiguity-routing-drift-2026-06-24.md`). The spec-refiner guarded the `awaiting_human` transition on the Ambiguities section ending with a question-mark character. The contract says any non-empty section should trigger the channel. List-style ambiguities (e.g. "- the auth model is unclear because of X") fell through to `build` and the human never saw the issue. ## Change Single-file, 8 insertions, 1 deletion in `src/damascus/phases.py`. Replaces the trailing-`?` regex check with a non-empty-after-strip check on the parsed section. ```python # Before: if "## Ambiguities" in text and re.search(r"\?\s*$", _section(text, "Ambiguities")): # After: ambiguities_section = _section(text, "Ambiguities") if "## Ambiguities" in text and ambiguities_section.strip(): ``` ## Verification - Contract test `test_refine_spec_routes_non_empty_ambiguities_to_awaiting_human` (PR #11): **PASS** (was RED on broken code). - Full contract suite: **19/19 pass**. - Unit suite: **10/10 pass**. The contract test is the structural counterpart: it source-greps for the buggy regex literal and asserts it's gone, plus the positive checks for `awaiting_human` routing and `open_human_issue` side effect. ## Why this is gap-finding-discipline compliant - Code-level decision in code PR, with the gap note documenting the rationale, options, and the recommended Option A. - The fix is the smallest possible shape (1 condition change), no architectural choice, no follow-up work. - Companion contract test (PR #11) was the visible-in-CI red that justified opening this PR — without it, the fix is a 1-line change with no spec. ## Out of scope - The related spec-refiner file-scope/budget_cycles drift is in PR #10 (a contract test only; the runtime fix needs an LLM session per the skill). - §7 Metrics/Analyzer still gated on issue #6 BMAD picks. ## Self-review Heartbeat authored, heartbeat cannot self-approve. Kay's call to merge. The fix is small, the contract test backs it, the gap note documents the rationale.
kaykayyali added 2 commits 2026-06-24 07:49:12 +00:00
test(contract): spec-refiner must route ANY non-empty Ambiguities to awaiting_human
Some checks failed
test / contract-and-unit (pull_request) Failing after 12s
Details
bb96bdb83f 
The contract at wiki/concepts/spec-refiner-contract.md says a non-empty
`## Ambiguities` section routes the row to phase=awaiting_human and
opens a human_issue. The current implementation in phases.py:74 guards
on re.search(r"\?\s*$", _section(text, "Ambiguities")) — this
requires the section to end with `?`, so a section that lists an
ambiguity without a trailing question mark falls through to build.

This is the structural counterpart to the E2E test that runs the
spec-refiner on a synthetic ambiguous spec and asserts the row lands
in awaiting_human regardless of trailing punctuation. Both tests are
RED today; both go green on the same fix (the documented Option A in
wiki/queries/damascus-orchestrator/spec-refiner-ambiguity-routing-drift-2026-06-24.md:
drop the `?` check, replace with a non-empty check on the section).

Uses re.escape() on the bug pattern so the literal text `\?\s*$`
in source matches character-for-character (raw-string regex would
interpret \? as literal ? and \s as whitespace class, false-passing
on broken code per the skill's Source-grep gotcha).

Co-Authored-By: Claude <noreply@anthropic.com>
fix(spec): route any non-empty Ambiguities section to awaiting_human
All checks were successful
test / contract-and-unit (pull_request) Successful in 12s
Details
abc8f4a0c7 
The spec-refiner at src/damascus/phases.py:74 guarded the awaiting_human
transition on a trailing question-mark character. The contract at
wiki/concepts/spec-refiner-contract.md says the trigger is any non-empty
section. A spec whose Ambiguities section lists an open question without
ending it in '?' (e.g. '- the auth model is unclear because of X') fell
through to phase='build' with the human never seeing the issue.

Replaces the regex check with a non-empty-after-strip check. Verified by:
- contract test (PR #11): test_refine_spec_routes_non_empty_ambiguities_to_awaiting_human
  now passes (RED on broken code, GREEN on this fix).
- full contract suite: 19/19 pass.
- unit suite: 10/10 pass.

Gap note: wiki/queries/damascus-orchestrator/spec-refiner-ambiguity-routing-drift-2026-06-24.md

Co-Authored-By: Claude <noreply@anthropic.com>
kaykayyali commented 2026-06-24 07:49:39 +00:00
Author
Owner
Copy Link

Self-review (heartbeat tick)

Diff walk

diff --git a/src/damascus/phases.py b/src/damascus/phases.py
@@ -71,7 +71,14 @@ def refine_spec(cur, item: dict) -> dict:
             "output_tokens": result["output_tokens"], "usd": result["usd"],
         })

-    if "## Ambiguities" in text and re.search(r"\?\s*$", _section(text, "Ambiguities")):
+    # Per spec-refiner-contract.md §3: any non-empty `## Ambiguities` section
+    # triggers the awaiting_human channel. The previous implementation required
+    # the section to end with a question-mark character, which silently
+    # swallowed list-style ambiguities (e.g. "- the auth model is unclear
+    # because of X") and routed them to build with the human never seeing
+    # the issue.
+    ambiguities_section = _section(text, "Ambiguities")
+    if "## Ambiguities" in text and ambiguities_section.strip():
         issue_id = state.open_human_issue(

8 insertions, 1 deletion. Single condition change. The ambiguities_section local is computed once and reused on the next line for state.open_human_issue's f-string argument (no behavioral change there — that's _section(result['text'], 'Ambiguities'), same value).

Contract walk

wiki/concepts/spec-refiner-contract.md §3 says: "On ambiguity: row transitions spec → awaiting_human, human_issues row is created with the question text." The trigger condition in the contract is "any non-empty ## Ambiguities section." The previous regex \?\s*$ narrowed this to "ends with ?" — a subset that swallows list-style ambiguities.

The fix removes the regex. The new check .strip() is non-empty-after-whitespace, which matches the contract's "non-empty" semantics. Whitespace-only sections (template stubs) are still treated as empty.

Test walk

  • PR #11 contract test (test_refine_spec_routes_non_empty_ambiguities_to_awaiting_human) — source-greps for the literal bug pattern (6 chars: \, ?, \, s, *, $) inside refine_spec. PASS on this branch. RED on main. This is the structural counterpart to the runtime fix.
  • Full contract suite: 19/19 pass.
  • Full unit suite: 10/10 pass.

Pitfalls avoided

  • Comment reproduces the bug pattern: my first attempt at the comment included the literal \?\s*$ string. The contract test source-greps the whole refine_spec body (including comments), so the test failed RED even on the fix. Fixed by paraphrasing the bug description in plain English. Caught by running the test before committing.
  • No force-push: branch was pushed as a fresh fix/spec-refiner-ambiguity-routing from a single commit on top of main. No rebase, no update-ref hack needed.
  • Heartbeat can't self-approve: per the tea pulls approve pitfall, the bot authored this PR and cannot approve it. Posted self-review here; merge is Kay's call.

Risk

Low. The change widens the ambiguity trigger (more specs go to awaiting_human than before). A spec whose Ambiguities section is now non-empty but was previously silently ignored will surface a human_issue to the operator instead of falling through to a doomed build. This is the intended behavior per the contract.

The reverse — a spec that previously routed correctly via the ? check — still routes correctly via .strip(). No regression.

Recommendation

Merge. Single-line condition change, contract-aligned, contract-tested, gap-noted.

## Self-review (heartbeat tick) ### Diff walk ``` diff --git a/src/damascus/phases.py b/src/damascus/phases.py @@ -71,7 +71,14 @@ def refine_spec(cur, item: dict) -> dict: "output_tokens": result["output_tokens"], "usd": result["usd"], }) - if "## Ambiguities" in text and re.search(r"\?\s*$", _section(text, "Ambiguities")): + # Per spec-refiner-contract.md §3: any non-empty `## Ambiguities` section + # triggers the awaiting_human channel. The previous implementation required + # the section to end with a question-mark character, which silently + # swallowed list-style ambiguities (e.g. "- the auth model is unclear + # because of X") and routed them to build with the human never seeing + # the issue. + ambiguities_section = _section(text, "Ambiguities") + if "## Ambiguities" in text and ambiguities_section.strip(): issue_id = state.open_human_issue( ``` 8 insertions, 1 deletion. Single condition change. The `ambiguities_section` local is computed once and reused on the next line for `state.open_human_issue`'s f-string argument (no behavioral change there — that's `_section(result['text'], 'Ambiguities')`, same value). ### Contract walk wiki/concepts/spec-refiner-contract.md §3 says: *"On ambiguity: row transitions `spec → awaiting_human`, `human_issues` row is created with the question text."* The trigger condition in the contract is "any non-empty `## Ambiguities` section." The previous regex `\?\s*$` narrowed this to "ends with `?`" — a subset that swallows list-style ambiguities. The fix removes the regex. The new check `.strip()` is non-empty-after-whitespace, which matches the contract's "non-empty" semantics. Whitespace-only sections (template stubs) are still treated as empty. ### Test walk - **PR #11 contract test** (`test_refine_spec_routes_non_empty_ambiguities_to_awaiting_human`) — source-greps for the literal bug pattern (6 chars: `\`, `?`, `\`, `s`, `*`, `$`) inside `refine_spec`. PASS on this branch. RED on main. This is the structural counterpart to the runtime fix. - **Full contract suite**: 19/19 pass. - **Full unit suite**: 10/10 pass. ### Pitfalls avoided - **Comment reproduces the bug pattern**: my first attempt at the comment included the literal `\?\s*$` string. The contract test source-greps the whole `refine_spec` body (including comments), so the test failed RED even on the fix. Fixed by paraphrasing the bug description in plain English. Caught by running the test before committing. - **No force-push**: branch was pushed as a fresh `fix/spec-refiner-ambiguity-routing` from a single commit on top of `main`. No rebase, no update-ref hack needed. - **Heartbeat can't self-approve**: per the `tea pulls approve` pitfall, the bot authored this PR and cannot approve it. Posted self-review here; merge is Kay's call. ### Risk Low. The change widens the ambiguity trigger (more specs go to `awaiting_human` than before). A spec whose Ambiguities section is now non-empty but was previously silently ignored will surface a `human_issue` to the operator instead of falling through to a doomed build. This is the intended behavior per the contract. The reverse — a spec that previously routed correctly via the `?` check — still routes correctly via `.strip()`. No regression. ### Recommendation Merge. Single-line condition change, contract-aligned, contract-tested, gap-noted.
kaykayyali merged commit efb576e865 into main 2026-06-24 13:09:26 +00:00
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
No items
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
kaykayyali
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: kaykayyali/damascus-orchestrator#12
Delete Branch "fix/spec-refiner-ambiguity-routing"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?