feat(agent): render public-safe scenario summaries - #416
Conversation
|
Note Gittensory Gate skippedPR closed before full evaluation. No late first comment was created.
Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers. |
JSONbored
left a comment
There was a problem hiding this comment.
@claytonlin1110 the renderer itself is better now, including pending PR scenario output.
A few notes:
- The remaining blocker is wiring:
renderPublicScenarioSummary()is only referenced by its unit test. - #271 is a parent mini-epic and explicitly says not to implement the full simulator in one PR. This looks like the public-summary child slice, not a full close of #271.
- A pure helper plus tests is not enough if no API, MCP, CLI, or control-panel path can use it.
Required changes:
- Wire the renderer into the intended real runtime surface, or narrow the PR/issue claim to the exact child issue it completes.
- Add tests for the production path that consumes the rendered summary.
- Update the PR closing reference so it does not close the parent mini-epic unless the full epic is complete.
Validation expected:
- Rerun scenario-summary tests plus the tests for the runtime surface you wire.
JSONbored
left a comment
There was a problem hiding this comment.
@claytonlin1110 this still should not merge as-is.
A few notes:
- The public/private boundary inside the renderer is pointed in the right direction.
- The blocker is that this is still an isolated renderer plus tests. renderPublicScenarioSummary is not wired into an API, MCP tool, agent path, UI route, or other production surface.
- The PR also claims the parent scenario epic, but this only implements one helper-sized slice.
Required changes:
- Wire renderPublicScenarioSummary into the real runtime surface that will emit or consume scenario summaries.
- Add behavior tests for that production path, not just the pure helper.
- Retarget the PR to the specific child issue/slice, or stop claiming that it closes the parent epic.
Validation expected:
- focused scenario-summary tests
- runtime-path tests proving the summary is actually used
- full validate
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
This reverts commit 5349657.
JSONbored
left a comment
There was a problem hiding this comment.
@claytonlin1110 the implementation is much closer now, but the issue claim still needs to be corrected.
A few notes:
renderPublicScenarioSummary()is wired intobuildLocalBranchAnalysis; this is no longer just dead helper code.- The sanitizer guard and scenario-summary tests are useful and aligned with the public-safe summary slice.
- The blocker is issue integrity: #271 is a mini-epic and says not to implement the full simulator in one PR. This branch appears to complete the public-safe scenario summary child slice, not the whole parent epic.
Required changes:
- Stop claiming
Closes #271. - Retarget the PR body to the focused child issue it actually resolves, likely the public-safe scenario-summary slice, or explain the no-close rationale if that child issue is not the intended target.
- Keep the parent epic open for the remaining simulator work.
Validation expected:
- No new source validation required if only the PR body/linked issue claim changes.
- If any code changes are made, rerun the focused scenario/local-branch tests and typecheck.
JSONbored
left a comment
There was a problem hiding this comment.
The implementation is in good shape now. renderPublicScenarioSummary is wired into a real runtime surface — buildLocalBranchAnalysis adds scenarioSummary to LocalBranchAnalysis and the new local-branch tests exercise it through that production path, so it is no longer an isolated helper. The public/private boundary is well handled: every text field passes sanitizePublicComment, the assertPublicSummaryClean guard rejects any serialization containing forbidden language, and there are forbidden-term assertions in both the scenario-summary and local-branch tests. Code-wise this is close to mergeable.
One required change remains — issue-claim integrity (no source change needed):
- The PR body still says
Closes #271, but #271 is the parent scenario mini-epic that explicitly says not to implement the full simulator in one PR. This branch implements only the public-safe scenario-summary child slice. Closing #271 on merge would prematurely close the epic and lose the remaining simulator work. - Please either retarget the closing reference to the specific child issue this resolves (the public-safe scenario-summary slice) or drop the
Closeskeyword and reference #271 withRefs #271, leaving the epic open.
No re-validation is required if only the PR body / linked-issue claim changes. If you touch source, rerun the focused scenario-summary and local-branch tests plus typecheck.
A couple of small, optional code notes for whenever you next push (not blocking):
- In
buildLocalBranchAnalysis,pendingDetectionForSummaryalways setsclassified: [], soextractPendingPullRequestsreturns nothing on this path — intentional? If the per-PR list is meant to surface here, the classified entries need to be threaded in; if not, a one-line comment noting it is summary-notes-only would help future readers. extractPendingScenarioNotesde-dupes vianew Set, which is fine, but worth confirming note ordering is stable for the deterministic-output expectation.
|
reviewbot · advisory review Reviewed 4 changed file(s) — two independent AI reviewers. Suggested action: 🛠️ Request changes. (reviewers split: request changes / merge) Address the suggestions below before merging. Reviewer A · Suggestions
Worth double-checking
Reviewer B · Suggestions
Worth double-checking
|
|
gittensory · advisory review Reviewed 4 changed file(s) — two independent AI reviewers. Suggested action: 🛠️ Request changes. (reviewers split: request changes / merge) Address the suggestions below before merging. Reviewer A · Suggestions
Worth double-checking
Reviewer B · Suggestions
Worth double-checking
|
Summary
src/scenarios/scenario-summary.tswithrenderPublicScenarioSummary(), converting outputs from the existing pressure, eligibility, and blocker simulators into a structuredPublicScenarioSummaryfor MCP/API and control-panel consumerssanitizePublicCommentwith a final defensive guard rejecting any serialization still containing forbidden language (wallet, hotkey, score estimates, trust scores, private reviewability)test/unit/scenario-summary.test.tswith 28 tests covering ranked option rendering, eligibility notes, blocker notes, data classification, combined inputs, sanitizer fixtures, and advisory-only invariantsScope
CONTRIBUTING.mdand does not reintroduce GitHub Pages, VitePress,site/, orCNAME.Validation
git diff --checknpm run actionlintnpm run typechecknpm run test:coveragelocally; global coverage stays at or above 97% for lines, statements, functions, and branches (aim for 98%+ branch coverage locally so CI variance does not fail near the threshold)npm run test:workersnpm run build:mcpnpm run test:mcp-packnpm run ui:openapi:checknpm run ui:lintnpm run ui:typechecknpm run ui:buildnpm audit --audit-level=moderateIf any required check was skipped, explain why:
npm run test:coveragesuite has 3 pre-existing failures on Windows (mcp-cli.test.ts,github-type-label.test.ts,mcp-release.test.ts) caused by a libuv platform assertion unrelated to this PR. All 96 scenario-related tests pass. Remaining CI checks (actionlint,build:mcp,ui:*,audit) are left for CI to run.Safety
Notes
renderPublicScenarioSummary()is composable: all inputs are optional, so callers can pass any subset of simulator outputs (pressure simulation, eligibility plan, blockers, scenario input) without requiring all of them.assertPublicSummaryCleanguard at the end of the render function is a defensive belt-and-suspenders check; in practice all fields are already sanitized individually before reaching it.