ci(#6788): add dummy-playback runtime for replaying canned results - #6789
ci(#6788): add dummy-playback runtime for replaying canned results#6789fullsend-ai-coder[bot] wants to merge 2 commits into
Conversation
|
🤖 Review · Commit: |
|
🤖 Review · Commit: |
f694211 to
fa60d62
Compare
|
🤖 Finished Review · ✅ Success · Started 2:42 AM UTC · Completed 3:01 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $8.22 |
|
Risk Assessment: moderate (2/5) DetailsLarge line count (2103) inflated by two brand-new files carrying zero churn risk. Changes to existing production files are minimal (1-5 lines each). Additive single-case additions following established patterns. CI workflow changes are simplifications/deletions. Bot author with well-scoped feature issue. Score unchanged from prior assessment. Previous runRisk Assessment: moderate (2/5) DetailsLarge line count (2283) inflated by two brand-new files carrying zero churn risk. Changes to existing production files are minimal (1-5 lines each). Additive single-case additions following established patterns. Test-infrastructure only with no production runtime impact. Bot author. Moderate risk from touching several hot-spot files simultaneously, offset by small footprint in each. Score unchanged from prior assessment. Previous run (2)Risk Assessment: moderate (2/5) DetailsLarge line count (1425) inflated by two brand-new files carrying zero churn risk. Changes to existing production files are minimal (1-5 lines each). Additive single-case additions following established patterns. Test-infrastructure only with no production runtime impact. Bot author. Moderate risk from touching several hot-spot files simultaneously, offset by small footprint in each. Score unchanged from prior assessment. Previous run (3)Risk Assessment: moderate (2/5) DetailsLarge line count (1400) inflated by two brand-new files carrying zero churn risk. Changes to existing production files are minimal (1-5 lines each). Additive single-case additions following established patterns. Test-infrastructure only with no production runtime impact. Bot author. Moderate risk from touching several hot-spot files simultaneously, offset by small footprint in each. Score unchanged from prior assessment. Previous run (4)Risk Assessment: moderate (2/5) DetailsLarge line count (1363) inflated by two brand-new files that carry zero churn risk. Changes to existing production files are minimal (1-5 lines each). Additive single-case additions following established patterns. Test-infrastructure only with no production runtime impact. Bot author. Moderate risk from touching several hot-spot files simultaneously, offset by small footprint in each. Previous run (5)Risk Assessment: moderate (2/5) DetailsLarge line count (1287) is inflated by two brand-new files (dummy_playback.go + test) that carry zero churn risk. Changes to existing production files are minimal (3-5 lines each). Those files are high-churn hotspots, but the edits are additive single-case additions following established patterns. The feature is test-infrastructure only with no production runtime impact. Bot author. Moderate risk stems from touching several hot-spot files simultaneously, but the small footprint in each limits blast radius. Previous run (6)Risk Assessment: moderate (2/5) DetailsLarge line count (1287) is inflated by two brand-new files (dummy_playback.go + test) that carry zero churn risk. Changes to existing production files are minimal (3-5 lines each). Those files are high-churn hotspots, but the edits are additive single-case additions following established patterns. The feature is test-infrastructure only with no production runtime impact. Bot author. Moderate risk stems from touching several hot-spot files simultaneously, but the small footprint in each limits blast radius. Previous run (7)Risk Assessment: moderate (2/5) DetailsLine count is high (1225) but almost entirely from two new files; changes to existing core files total ~12 lines of trivial registration plumbing. Test coverage for the new code is substantial (723 lines, 0.33 ratio). Well-scoped feature with clear acceptance criteria, no protected/security paths, additive-only change, and low issue risk. Consistent with prior assessment. Previous run (8)Risk Assessment: moderate (2/5) DetailsLine count is high (1288) but almost entirely from two new files; changes to existing core files total ~15 lines of trivial registration plumbing. Test coverage for the new code is substantial (720 lines). Well-scoped feature with clear acceptance criteria, no protected/security paths, and low git-history risk. Prior assessment of 2/moderate confirmed — no signals warrant change. Previous run (9)Risk Assessment: moderate (2/5) DetailsLine count is high (1287) but almost entirely from two new files; changes to existing core files total only ~15 lines of trivial registration plumbing. Test coverage for the new code is substantial (720 lines). Well-scoped feature with clear acceptance criteria and no protected/security paths. Prior assessment of 2/moderate confirmed — Tier 1 signals unchanged, Tier 2/3 provide no reason to adjust. Previous run (10)Risk Assessment: moderate (2/5) DetailsLine count is high (1283) but almost entirely from two new files; changes to existing core files total only ~15 lines of trivial registration plumbing. Test coverage for the new code is substantial (720 lines). Well-scoped feature with clear acceptance criteria and no protected/security paths. Overall moderate risk driven by new code volume, offset by minimal existing-code churn and strong test presence. Previous run (11)Risk Assessment: moderate (2/5) DetailsLine count is high (1267) but almost entirely from two new files; changes to existing core files total only 6 lines of trivial registration plumbing. Test coverage for the new code is substantial (720 lines). Well-scoped feature with clear acceptance criteria and no protected/security paths. Overall moderate risk driven by new code volume, offset by minimal existing-code churn and strong test presence. Previous run (12)Risk Assessment: moderate (2/5) DetailsLine count is high (1207) but almost entirely from two new files. Changes to existing core files total only 6 lines of trivial registration plumbing. Test coverage for the new code is substantial (677 lines). Well-scoped feature with clear acceptance criteria. Overall moderate risk driven by new code volume, offset by minimal existing-code churn and strong test presence. Previous run (13)Risk Assessment: moderate (2/5) DetailsLine count is high (1120) but almost entirely from two new files. Changes to existing core files total only 6 lines of trivial registration plumbing. Test coverage for the new code is substantial (662 lines). Well-scoped feature with clear acceptance criteria. Overall moderate risk driven by new code volume, offset by minimal existing-code churn and strong test presence. |
ReviewFindingsHigh
Low
Next steps:
Previous runReviewFindingsCritical
High
Medium
Low
Next steps:
Previous run (2)ReviewFindingsLow
Previous run (3)ReviewFindingsLow
Next steps:
Previous run (4)ReviewFindingsLow
Next steps:
Previous run (5)ReviewFindingsLow
Next steps:
Previous run (6)ReviewFindingsMedium
Low
Next steps:
Previous run (7)ReviewFindingsMedium
Low
Next steps:
Previous run (8)ReviewFindingsLow
Next steps:
Previous run (9)ReviewFindingsLow
Next steps:
Previous run (10)ReviewFindingsLow
Next steps:
Previous run (11)ReviewFindingsMedium
Low
Next steps:
Previous run (12)ReviewFindingsMedium
Low
Next steps:
Previous run (13)ReviewFindingsMedium
Low
Next steps:
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
ggallen
added
fullsend-fix
|
/fs-fix - rebase, fix all review issues, respond to and resolve all conversations. |
1 similar comment
|
/fs-fix - rebase, fix all review issues, respond to and resolve all conversations. |
|
🤖 Finished Fix · ✅ Success · Started 3:41 AM UTC · Completed 3:53 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $4.24 |
- Validate playback-comment-url path starts with "/" to prevent argument injection via gh/glab CLI flags (both two-line and legacy single-line formats) - Add dummy-playback row to docs/runtimes.md runtime summary table - Add Dummy-playback runtime operations section to docs/contributing/runtime-implementation.md with security matrix note, playlist format, companion files, fix/feature entries, tracking comment, and config key table - Update ValidRuntimes() comment to mention dummy-playback alongside dummy - Group top-level constants into a const() block - Move dummy-playback case adjacent to dummy in registry.go - Add 3 test cases for argument injection prevention Addresses review feedback on #6789
🔧 Fix agent — iteration 1 (human-triggered)Addressed all review findings from the review agent. Fixed argument injection vulnerability in parsePlaybackCommentRef by validating path starts with '/'. Added documentation: dummy-playback row in runtimes.md table, full operations section in runtime-implementation.md. Updated ValidRuntimes() comment, grouped constants, and reordered registry cases. Disagreed on 3 findings: PR body editing (out of scope), commit prefix (human-authored commit), and field doc style (reviewer agreed inline is adequate). Also stripped Signed-off-by trailers from agent commits that were flagging the pre-commit gate. Fixed (7):
Disagreed (3):
Tests: passedNext steps:
|
fa60d62 to
80d5903
Compare
|
🤖 Review · ❌ Terminated · Started 3:54 AM UTC · Ended 4:13 AM UTC Commit: |
Site previewPreview: https://74998e70-site.fullsend-ai.workers.dev Commit: |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
🤖 Finished Review · ✅ Success · Started 3:54 AM UTC · Completed 4:13 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $9.22 |
|
🤖 Finished Fix · ✅ Success · Started 4:15 AM UTC · Completed 4:25 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $2.45 |
Superseded by updated review
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
the
ready-for-merge
|
/fs-fix Rebase. |
|
🤖 Finished Fix · ✅ Success · Started 6:33 PM UTC · Completed 6:53 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $5.52 |
Add a dummy-playback runtime that replays canned agent results from an ordered playlist without LLM inference for behaviour tests. Includes playlist management, companion file copying, playback comment tracking via forge API, and review metadata injection. Make readPlaybackComment and updatePlaybackComment testable by injecting a forgeAPIFunc, bringing patch coverage above the 80% threshold. Addresses #6789 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
🔧 Fix agent — iteration 6 (human-triggered)Validation retry: the previous iteration's commit contained Signed-off-by trailers which are not allowed for agent commits. Soft-reset the commit and recommitted with the same changes and message but without the Signed-off-by trailers, keeping only the Co-Authored-By trailer. Fixed (3):
Tests: passedNext steps:
|
341e2e5 to
5ddc31c
Compare
|
🤖 Finished Review · ✅ Success · Started 6:55 PM UTC · Completed 7:17 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $7.17 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
removed
the
ready-for-merge
|
🤖 Finished Fix · ✅ Success · Started 7:19 PM UTC · Completed 7:28 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $2.35 |
🔧 Fix agent — iteration 2 (bot-triggered)Restored the stage-aware role check (observation stages accept triage role, ADR 0054) and entity-author bypass for work_item entities in routeSlashCommand, along with 154 lines of test coverage. These were inadvertently reverted by the PR commit. Protected-path findings (.github/workflows/) require human approval. Low-severity findings (unused export, prefix style) are intentional design choices. Fixed (1):
Disagreed (4):
Tests: passed Decision points
Next steps:
|
|
🤖 Finished Review · ✅ Success · Started 7:30 PM UTC · Completed 8:11 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $10.90 |
|
🤖 Finished Fix · ✅ Success · Started 8:13 PM UTC · Completed 8:21 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $1.22 |
🔧 Fix agent — iteration 3 (bot-triggered)All actionable review findings are already addressed in the current code from previous iterations. The three stale-reference findings (run.go:459, run.go:164, docs/runtimes.md harness config table) were fixed in prior iterations but re-raised by the reviewer against an older HEAD. The protected-path findings require human approval. The symlink-race and env-var-trust findings are explicitly acknowledged as negligible by the reviewer in a test-only runtime. Disagreed (8):
Tests: passed
Decision points
Next steps:
|
1 participant
Summary
Add a
dummy-playbackruntime that replays canned agent results from an ordered playlist without LLM inference. This enables fast, deterministic e2e tests that exercise the full dispatch→pre-script→runtime→post-script pipeline using pre-recorded output.Changes
internal/runtime/dummy_playback.go—DummyPlaybackRuntimestruct implementing theRuntimeinterface. Reads from.fullsend/results/playlist.yaml, serves the current entry'sresult.jsonto the sandbox output directory, copies companion files, creates feature branches or commits fixes, injects review metadata, and advances the playlist position.internal/runtime/dummy_playback_test.go— Unit tests covering: playlist loading (valid, missing, invalid YAML), result serving with playlist advancement, empty playlist, playlist exhaustion, missing result file, path traversal rejection, CLI allowlist, companion files, bootstrap, and clear iteration artifacts.internal/runtime/registry.go— Registerdummy-playbackinResolve().internal/runtime/runtime.go— AddForgefield toRunParamsfor GitLab support.internal/config/config.go— Adddummy-playbacktoValidRuntimes().internal/cli/run.go— PassforgePlatformasForgeonRunParams.Testing
go build ./...passesgo test ./internal/runtime/...passes with new unit testsgo vet ./...cleanCloses #6788