Restore ledger push handoff and correct smoke projection checks - #65405
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The zero-message ledger path still omits the required transaction artifact, and the unrelated skill edit removes work-queue routing.
Review effort: Balanced
Findings: 1
Open (2)
What changed in this PR
Restores standalone ledger transaction handoff and makes ledger smoke checks use persisted, cross-run projections.
Changes:
- Adds standalone ledger definitions to safe-output handler configuration with regression coverage.
- Adds Python SQLite runtimes and corrects smoke-test projection expectations.
- Regenerates all affected workflow lock files.
| File | Description |
|---|---|
pkg/workflow/safe_outputs_config_runtime.go |
Wires ledger handlers into runtime configuration. |
pkg/workflow/ledger_test.go |
Verifies handler ledger definitions. |
.github/workflows/smoke-repo-memory-ledger.md |
Updates runtime and cross-run checks. |
.github/workflows/smoke-repo-memory-ledger.lock.yml |
Regenerates the workflow. |
.github/workflows/smoke-builtin-ledgers.md |
Adds Python-based SQLite checks. |
.github/workflows/smoke-builtin-ledgers.lock.yml |
Regenerates the workflow. |
.github/workflows/daily-mcp-concurrency-analysis.lock.yml |
Adds ledger handler configuration. |
.github/workflows/daily-caveman-optimizer.lock.yml |
Adds ledger handler configuration. |
.github/workflows/daily-awf-spec-compiler-surfacing.lock.yml |
Adds ledger handler configuration. |
.github/workflows/copilot-centralization-optimizer.lock.yml |
Adds ledger handler configuration. |
.github/workflows/audit-workflows.lock.yml |
Adds ledger handler configuration. |
.github/skills/agentic-workflows/SKILL.md |
Reorders one reference but removes work-queue routing. |
| if handlerConfig := buildLedgerRequestCompactionHandlerConfig(data.LedgerConfig); handlerConfig != nil { | ||
| config[ledgerRequestCompactionHandlerKey] = handlerConfig | ||
| } | ||
| addStandaloneLedgerConfigs(config, data.LedgerConfig) |
There was a problem hiding this comment.
Implemented in actions/setup/js/safe_output_handler_manager.cjs: empty-message runs now finalize the configured ledger append handler and write the empty versioned transaction artifact. Added regression coverage. Commit: 6411e8a.
|
🧠 Matt Pocock Skills Reviewer failed during the skills-based review. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details.
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details.
|
|
✅ PR Code Quality Reviewer completed the code quality review. Review submission was blocked: submit_pull_request_review failed twice with a permission denial, so no GitHub write was emitted.
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
There was a problem hiding this comment.
Impeccable Review Summary
This is primarily a Go-backend/infra fix (no UI), so standard Impeccable UX modes do not apply directly. I ran a correctness/reliability review instead, prioritizing security > correctness > reliability > maintainability, and verified findings against the actual source.
Verification performed
go build ./...— passesgo test ./pkg/workflow/... -run TestStandaloneLedgerWiresValidationArtifactAndPersistenceJobs— passes- Recompiled
smoke-builtin-ledgers.mdandsmoke-repo-memory-ledger.md— generated.lock.ymlmatches committed files exactly (no diff) - Traced
safe_output_handler_manager.cjsmain() early-return path andledger_append.cjsfinalize() logic to confirm a pre-existing, still-open correctness gap
Findings
| # | Severity | File | Lines | Issue | Confidence |
|---|---|---|---|---|---|
| 1 | 🟠 HIGH | pkg/workflow/safe_outputs_config_runtime.go | 108 | Pre-existing, unaddressed gap: when a ledger-enabled run emits zero safe-output messages, safe_output_handler_manager.cjs returns before loadHandlers() runs, so ledger_append.finalize() never executes and ledger-transactions.json is never written — yet the safe-outputs job's artifact-upload step uses if-no-files-found: error, and buildPushLedgerChangesJob requires that artifact. This will fail any ledger-enabled run with no other safe outputs. |
7/10 |
| 2 | 🟡 MEDIUM | .github/skills/agentic-workflows/SKILL.md | ~93-109 | Reordering work-queue.md in the file list and re-adding its routing bullet is fine, but double-check this doesn't silently drop/duplicate the entry relative to upstream source:-managed content if this SKILL.md is provenance-managed elsewhere. |
4/10 |
Both items were flagged by prior automated review comments already on this PR and remain valid/unresolved by the current diff — I did not duplicate them as new inline comments.
Scope-checked changes (no new issues)
pkg/workflow/safe_outputs_config_runtime.go: correctly reusesaddStandaloneLedgerConfigs(shared withgenerateSafeOutputsConfig) instead of only callingbuildLedgerRequestCompactionHandlerConfig, fixing the actual PR regression (missingledger_appendkey inGH_AW_SAFE_OUTPUTS_HANDLER_CONFIG). Verified via the new/updated test.pkg/workflow/ledger_test.go: new assertion correctly validatesledger_append.ledgersis present and matches betweenconfig.jsonand the handler-manager env var.- Smoke workflow
.mdchanges (Python runtime addition,cloud-hypervisorremoval, projection-timing guidance correction) are internally consistent and match the.lock.ymldiffs; recompilation confirms no drift.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 131 AIC · ⌖ 13.3 AIC · ⊞ 8.1K
|
@copilot run pr-finished skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot fix chi failures |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed the stale JavaScript token-field and Go workflow-model test expectations in
Ran the PR finisher pass. Local formatting, lint, Go tests, JavaScript tests, and workflow recompilation pass. The PR is not merge-ready yet: its prior Go custom-lint check reports 294 unchecked write results across 27 files, and an in-scope review thread remains unresolved. CI on the pushed HEAD also needs a maintainer re-trigger. Test corrections are in |


Ledger pushes failed because the safe-output job did not load the ledger handler, leaving
push_ledger_changeswithout a transaction artifact. Ledger smokes also lacked a usable SQLite query runtime and expected new writes in a projection created before those writes were persisted.