Skip to content

✨ Add workflow control token syntax#55

Open
sdougbrown wants to merge 3 commits into
mainfrom
feat/workflow-markers
Open

✨ Add workflow control token syntax#55
sdougbrown wants to merge 3 commits into
mainfrom
feat/workflow-markers

Conversation

@sdougbrown

@sdougbrown sdougbrown commented Jun 16, 2026

Copy link
Copy Markdown
Owner

Summary

  • add canonical <|workflow/status/team|> control token parsing
  • keep legacy [loop], [status], and [team: skip] markers for compatibility
  • update docs, templates, and tests to prefer the new token syntax

Closes #41

Tests

  • go test ./internal/digest ./internal/teamrunner ./internal/cli ./internal/looprunner
  • go test ./...

@umpire-bot umpire-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is marked... FOUL BALL. 👉

Warnings

  • Duplicate Documentation: docs/team.md:66 contains a duplicate H2 section "## How a team run works".
  • Logic Coupling: internal/digest/chunkbuf.go:80 uses a shared lastStatusKey for both ScanStatusAngle and ScanStatusMarker, risking incorrect suppression.
  • Code Duplication: internal/digest/loopmarker.go:15 duplicates phaseconfig.LoopDirectiveSeverity logic.
  • Test Duplication: internal/digest/chunkbuf_test.go:163 contains a test case nearly identical to TestChunkBufferLoopMarkerHighestSeverityWins.

Nits

  • Missing Test Case: internal/digest/chunkbuf_test.go:201 lacks a test for accumulating multiple angle tokens in the buffer.

Prior Findings Review

Fixed (2)

Prior findings that have been resolved.

  • internal/teamrunner/runner.go:631 — The use of `strings.TrimSpace(line)` is redundant because `teamSkipRe` is already anchored with `^\s*` and `\s*$`.
  • internal/digest/loopmarker.go:42 — The `continue` on a legacy `[loop: ...]` match makes the angle-token branch unreachable if both syntaxes appear on the same line. Add a comment to make this intentional precedence explicit.

Uncertain (1)

These prior findings could not be confidently classified against the current code.
A human should decide whether they remain relevant.

  • internal/digest/loopmarker.go:13

Comment thread internal/teamrunner/runner.go Outdated
Comment thread internal/teamrunner/runner.go
Comment thread internal/digest/loopmarker.go Outdated
Comment thread internal/digest/loopmarker.go Outdated
@sdougbrown

Copy link
Copy Markdown
Owner Author

👾 AI Agent

Addressed 4 inline comments across 2 files:

  • Fixed (3): removed redundant TrimSpace before teamSkipRe; loosened angle-token label/name parsing for | and >; added parser precedence clarification. Commit: 1a41806.
  • Accepted as-is (1): kept gofmt-only alignment in completePhaseRequirements; it has no behavior impact and reverting it would add churn.
  • Dismissed/clarified: the top-level “breaking change” warning is oversold — legacy [loop: ...], [status: ...], and [team: skip | ...] remain supported, while docs/templates now teach the new <|...|> syntax.

Review loop: 1 iteration. Tests passed:

  • go test ./internal/digest ./internal/teamrunner ./internal/cli ./internal/looprunner
  • go test ./...

@sdougbrown

Copy link
Copy Markdown
Owner Author

@umpire-bot review

Comment thread docs/team.md Outdated
// ScanStatusAngle runs ExtractStatusAngle over the accumulated buffer.
// It returns ok=false for a status angle token that was already returned by
// a previous call, preventing duplicate emissions.
func (cb *ChunkBuffer) ScanStatusAngle() (phase, label string, ok bool) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ScanStatusAngle and ScanStatusMarker share the lastStatusKey field in ChunkBuffer. This shared state could cause ScanStatusAngle to incorrectly suppress a valid marker if a ScanStatusMarker call was previously made with the same phase and label.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👾 AI Agent

Dismissed: this shared key is intentional. The canonical and legacy status syntaxes represent the same status update when phase+label match, so suppressing the duplicate is desired compatibility behavior. I also tightened ScanStatusAngle in 91eaa25 so it emits the first non-duplicate angle marker in stream order when multiple tokens are buffered.

Comment thread internal/digest/loopmarker.go
}
}

func TestChunkBufferAngleTokenLoopMarker(t *testing.T) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TestChunkBufferAngleTokenLoopMarker is structurally identical to TestChunkBufferLoopMarkerHighestSeverityWins. Consolidate these into a parameterized table test to reduce duplication.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👾 AI Agent

Dismissed: the tests are intentionally parallel, one protecting legacy [loop: ...] scanning and one protecting canonical <|workflow: ...|> scanning through the same streaming scanner. Consolidating them would make the canonical coverage less obvious for little gain.

Comment thread internal/digest/chunkbuf_test.go
@sdougbrown

Copy link
Copy Markdown
Owner Author

👾 AI Agent

Addressed 5 new comments across 3 files:

  • Fixed (3): removed the duplicate team docs section; added a sync comment for directive severity; expanded status-angle chunk-buffer coverage and adjusted scanning to emit the first non-duplicate marker in stream order. Commit: 91eaa25.
  • Dismissed (2): shared lastStatusKey is intentional compatibility dedup for equivalent canonical/legacy status markers; separate legacy-vs-canonical loop marker tests are deliberate coverage.

Review loop: 1 iteration. Tests passed:

  • go test ./internal/digest ./internal/teamrunner ./internal/cli ./internal/looprunner
  • go test ./...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Correct internal workflow marker prefix

1 participant