Add standalone safe-output-backed ledger configuration - #64354
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot ensure ledger changes are pushed to git upstream. Reuse repo-memory git helpers. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| const id = finalId(transactionId, index); | ||
| if (request.temp_id) mapping.set(`${ledger}:${request.temp_id}`, id); | ||
| normalized.push({ ledger, transaction_id: transactionId, record: { ...request.record, id } }); |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Core safe-output handling, projection generation, validation, artifact transfer, and durable Git persistence are not yet operational.
Review effort: Balanced
Findings: 6
Open (9)
What changed in this PR
Adds standalone tools.ledger configuration and scaffolding for safe-output-backed ledger persistence.
Changes:
- Parses ledger configuration, schemas, limits, and prompt guidance.
- Registers
ledger_appendwith temporary-ID normalization. - Adds a trusted
push_ledger_changesjob boundary.
| File | Description |
|---|---|
pkg/workflow/workflow_data.go |
Stores normalized ledger configuration. |
pkg/workflow/unified_prompt_step.go |
Adds ledger prompt guidance. |
pkg/workflow/tools_types.go |
Defines the ledger tool type. |
pkg/workflow/tools_parser.go |
Parses tools.ledger. |
pkg/workflow/safe_outputs_validation_config.go |
Defines append-request validation fields. |
pkg/workflow/safe_outputs_tools_computation.go |
Enables the append tool. |
pkg/workflow/safe_outputs_config_generation.go |
Generates append handler configuration. |
pkg/workflow/ledger.go |
Implements configuration and prompt generation. |
pkg/workflow/ledger_test.go |
Tests parsing and prompts. |
pkg/workflow/ledger_job.go |
Generates the persistence job. |
pkg/workflow/js/safe_outputs_tools.json |
Describes the append tool. |
pkg/workflow/jobs.go |
Registers the persistence job name. |
pkg/workflow/compiler_orchestrator_workflow.go |
Extracts configuration and rejects legacy usage. |
pkg/workflow/compiler_jobs.go |
Adds persistence and conclusion dependencies. |
pkg/parser/schemas/main_workflow_schema.json |
Allows standalone ledger frontmatter. |
actions/setup/js/push_ledger_changes.cjs |
Reads transaction artifacts and reports results. |
actions/setup/js/ledger_transactions.test.cjs |
Tests transaction normalization. |
actions/setup/js/ledger_transactions.cjs |
Normalizes IDs and references. |
| const id = finalId(transactionId, index); | ||
| if (request.temp_id) mapping.set(`${ledger}:${request.temp_id}`, id); | ||
| normalized.push({ ledger, transaction_id: transactionId, record: { ...request.record, id } }); |
There was a problem hiding this comment.
Addressed in fed4a69: the trusted normalizer validates configured schemas and record/patch limits, rejects reserved fields, and emits only normalized records.
| result.ledgers[name] = { | ||
| requested: ledger.appends.length, | ||
| validated: ledger.appends.length, | ||
| persisted: 0, |
There was a problem hiding this comment.
Implemented in cfb75e6 and wired in fed4a69: persistence hydrates and reconciles the ledger branch, commits with the repo-memory retry/CAS helpers, and reports durable counts only after the push succeeds.
| case "additionalProperties", "oneOf", "anyOf": | ||
| // These keywords are accepted by the existing JSON-schema validator. |
There was a problem hiding this comment.
Addressed in e938829: schema validation now walks the full tree, rejects expressions recursively, and compiles the schema before accepting it.
| func (c *Compiler) buildPushLedgerChangesJob(data *WorkflowData, threatDetectionEnabled bool) *Job { | ||
| needs := []string{string(constants.AgentJobName), string(constants.ActivationJobName)} | ||
| if IsDetectionJobEnabled(data.SafeOutputs) && threatDetectionEnabled { | ||
| needs = append(needs, string(constants.DetectionJobName)) |
There was a problem hiding this comment.
Addressed in fed4a69: push_ledger_changes depends on safe_outputs and downloads the versioned transaction artifact before reconciliation.
| if data.LedgerConfig != nil && data.LedgerConfig.Enabled() { | ||
| safeOutputsConfig["ledger_append"] = map[string]any{"max": 100} |
There was a problem hiding this comment.
Addressed in fed4a69: the safe-output manager registers ledger_append and writes the validated versioned transaction artifact.
| if data.LedgerConfig != nil && data.LedgerConfig.Enabled() { | ||
| enabledTools["ledger_append"] = struct{}{} |
There was a problem hiding this comment.
Addressed in e938829: standalone ledger configuration enables the safe-output pipeline, MCP setup, and ledger_append registration without requiring unrelated safe outputs.
| workflowData.LedgerConfig = toolsConfig.Ledger | ||
| if workflowData.RepoMemoryConfig != nil { | ||
| for _, memory := range workflowData.RepoMemoryConfig.Memories { | ||
| if memory.Ledger != nil { | ||
| return errors.New("tools.repo-memory.ledger is no longer supported; migrate to tools.ledger") |
There was a problem hiding this comment.
Addressed in e938829: the legacy ledger key is detected before repo-memory parsing so users receive the migration error consistently.
| if schema == "" || strings.HasPrefix(schema, "/") || strings.Contains(schema, "..") || strings.ContainsAny(schema, `\${{}`) { | ||
| return LedgerConfig{}, fmt.Errorf("tools.ledger.%s.schema must be a repository-relative path without expressions or traversal", name) | ||
| } | ||
| cfg.SchemaPath = schema |
There was a problem hiding this comment.
Addressed in e938829: repository-relative schemas are resolved, size-bounded, validated, and serialized into the trusted ledger configuration.
| var b strings.Builder | ||
| b.WriteString("Persistent ledgers available (SQLite is read-only and disposable):\n") | ||
| for _, ledger := range config.Ledgers { | ||
| fmt.Fprintf(&b, "- %s: %s\n", ledger.Name, filepath.Join(ledgerProjectionRoot, ledger.Name, "ledger.db")) |
There was a problem hiding this comment.
Addressed in fed4a69: the agent setup hydrates ledger branches and creates the read-only SQLite projection before execution.
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft.
|
|
✅ PR Code Quality Reviewer completed the code quality review. No GitHub review was submitted because safeoutputs create_pull_request_review_comment and submit_pull_request_review were both blocked with: Permission denied and could not request permission from user.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ 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
|
🏗️ ADR Required — draft added for PR #64354ResultThis PR requires ADR enforcement because it adds more than 100 new lines in business-logic directories ( Evidence used
Action takenI added a draft ADR here:
Next action for the authorReview the draft ADR, correct any missing rationale or trade-offs, and keep it with the implementation as the design record for this ledger architecture change.
|
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.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design — requesting changes. Existing bot/Copilot inline comments already flag the critical functional gaps (no real Git persistence in push_ledger_changes.cjs, no schema/limit enforcement in normalization, missing ledger_append handler wiring, and a nil-guard that skips ledger-only workflows in computeEnabledToolNames). This review adds three test-coverage and code-quality findings on top of those.
📋 Key Themes & Highlights
Key Themes
- Silent error swallowing:
NewToolsdiscardstools.ledgerparse errors instead of propagating them, so invalid configs can silently disable the ledger rather than fail the build — this diverges from theGitHub.reposParseErrpattern already established for surfacing config errors fromNewToolscallers. - Missing test coverage on the trusted job:
push_ledger_changes.cjs— the one file withcontents: writepermissions performing the actual (currently stubbed) persistence — ships with zero tests, unlike its siblingledger_transactions.cjs. - Duplicated branches:
parseLedgerToolConfig'ssingleandlen(root) == 0cases are identical and can be collapsed.
Positive Highlights
- ✅ Deterministic, hash-based temporary-ID generation with clear duplicate/unknown-ledger rejection in
ledger_transactions.cjs, backed by a solid test file. - ✅ Clean separation of the untrusted normalization step from the trusted persistence job boundary, matching the existing repo-memory architecture.
- ✅ Legacy
tools.repo-memory.ledgermigration is explicitly rejected with an actionable error message.
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 61.8 AIC · ⌖ 14.9 AIC · ⊞ 10K
Comment /matt to run again
| if val, exists := toolsMap["repo-memory"]; exists { | ||
| tools.RepoMemory = parseRepoMemoryTool(val) | ||
| } | ||
| if val, exists := toolsMap["ledger"]; exists { |
There was a problem hiding this comment.
[/tdd] NewTools silently discards tools.ledger parse errors: on failure it returns tools with Ledger unset instead of propagating the error, so an invalid tools.ledger config compiles as if ledger support were simply absent rather than failing loudly. Since ParseToolsConfig already validates this same config via parseLedgerToolConfig(raw) a few lines above (tools_types.go), the two error paths can disagree — a config could pass ParseToolsConfig but silently lose Ledger here if NewTools is called directly (e.g. mcp_cli_mount.go:183, tools.go:71, workflow_builder.go:70), none of which check for the swallowed error.
💡 Suggested fix
Mirror the GitHub.reposParseErr pattern already used for repo config: store the error on the Tools struct (e.g. tools.ledgerParseErr) so every caller path — not just ParseToolsConfig — can detect and surface it, or have NewTools itself return (*Tools, error).
A regression test asserting that an invalid tools.ledger config surfaces an error through every NewTools call site (not just ParseToolsConfig) would catch this class of silent-swallow bug.
@copilot please address this.
There was a problem hiding this comment.
Addressed in e938829: NewTools retains ledger parse errors and compiler validation surfaces them rather than silently dropping invalid configuration.
| single = true | ||
| } | ||
| } | ||
| if single { |
There was a problem hiding this comment.
[/codebase-design] The single and len(root) == 0 branches are identical (both build parseLedgerConfig(defaultLedgerName, root) and return early) — this duplication makes the two-forms-are-unambiguous logic harder to follow and easy to diverge on the next edit.
💡 Suggested simplification
if single || len(root) == 0 {
cfg, err := parseLedgerConfig(defaultLedgerName, root)
if err != nil {
return nil, err
}
result.Ledgers = []LedgerConfig{cfg}
return result, nil
}Collapsing the two branches keeps the "concise default ledger" special case in one place and reduces the surface for future logic drift between them.
@copilot please address this.
There was a problem hiding this comment.
Simplified in e938829: the concise default-ledger and empty configuration cases now share one branch.
| @@ -0,0 +1,58 @@ | |||
| // @ts-check | |||
There was a problem hiding this comment.
[/tdd] Unlike ledger_transactions.cjs, which ships ledger_transactions.test.cjs, this new trusted-persistence entry point has no test file at all — no coverage for readTransactions() parsing/validation, the per-ledger result shape, or main()'s GITHUB_OUTPUT write path.
💡 Suggested tests
readTransactions()returns the empty-ledger default whenGH_AW_LEDGER_TRANSACTIONSis unset.readTransactions()throws on malformed JSON, wrongversion, or a non-objectledgersfield.main()producespersisted: 0/reconciled: 0for a well-formed artifact today (documenting the current stub behavior) so a future PR that wires in real persistence has a clear regression baseline to update deliberately rather than by accident.
Given this file is the trusted job with contents: write permissions reconciling ledger branches, it's the highest-risk file in the PR to ship without any test coverage.
@copilot please address this.
There was a problem hiding this comment.
Addressed in fed4a69: added persistence tests covering transaction artifact validation and the trusted push path.
|
|
There was a problem hiding this comment.
Impeccable Review — distill/extract (refactor/new-feature: standalone ledger config)
This PR introduces a large surface (config parsing, safe-output tool, trusted persistence job) but several pieces are stubs or unwired, and existing automated reviewers (GHAS, Copilot) already flagged the most severe gaps on this PR:
push_ledger_changes.cjsnever actually persists anything (persisted: 0hard-coded, no git commit/push).ledger_appendhas no config-key-to-handler wiring insafe_output_handler_manager.cjs, so accepted requests are never processed.- No JSON Schema validation or
max-record-kb/max-patch-kbenforcement is applied to agent-submitted records before they're written into the "validated" transaction artifact. - The JSON-Schema keyword allowlist in
validateInlineLedgerSchemaacceptsoneOf/anyOf/additionalPropertieswithout recursing into their values, so${{ }}expressions can slip through inside those keywords. push_ledger_changesjob has no dependency onsafe_outputs/ no artifact-download step wiring toGH_AW_LEDGER_TRANSACTIONS, so it can't get the real transaction file.- The prompt (
buildLedgerPromptSection) advertises a SQLite projection file that nothing ever creates.
New finding from this review: pkg/workflow/tools_parser.go (NewTools, ~line 169) — an invalid tools.ledger config makes the function return tools immediately, silently skipping parsing of every tool declared after ledger (timeout, startup-timeout, cli-proxy, all custom MCP tools). Verified locally with a unit test reproducing the silent drop. Other tool parsers in the same function (e.g. bash) log-and-continue on invalid config; ledger should do the same instead of aborting the whole function.
Given the ledger feature is not yet functionally wired end-to-end (per the existing unresolved bot comments) and this new correctness bug affects unrelated tool parsing, requesting changes before merge.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 153.9 AIC · ⌖ 13.3 AIC · ⊞ 8.1K
| if val, exists := toolsMap["ledger"]; exists { | ||
| ledger, err := parseLedgerToolConfig(val) | ||
| if err != nil { | ||
| return tools |
There was a problem hiding this comment.
Bug: an invalid tools.ledger config causes NewTools to return tools immediately (line 169), silently skipping parsing of every tool listed after ledger in the function — timeout, startup-timeout, cli-proxy, and all custom MCP tools (the loop that populates tools.Custom never runs). Reproduced locally: with {"ledger": {"schema": "../bad.json"}, "timeout": 30, "cli-proxy": true, "custom-tool": {...}}, tools.Timeout, tools.CLIProxy, and tools.Custom all come back empty/nil even though only the ledger config is invalid. Other tool parsers in this function (e.g. bash) log a warning and continue instead of aborting the whole function. Fix by logging the ledger parse error (like the bash case) and falling through to continue parsing, rather than returning early.
@copilot please address this.
There was a problem hiding this comment.
Addressed in e938829: invalid ledger parsing records the error while continuing to parse the remaining tools; validation later surfaces the error.
|
🎉 Excellent work on the ledger redesign! The change has solid test coverage, a clear description, and a focused scope. It looks ready for review. 🚀
|
|
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Ran the pr-finisher pass. The local checks and security re-review are complete; the final patch is
The branch already included current |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in |
|
@copilot resolve the merge conflicts on this branch. |
|
|
…-ledger-design # Conflicts: # .github/workflows/copilot-centralization-optimizer.lock.yml # .github/workflows/daily-awf-spec-compiler-surfacing.lock.yml # .github/workflows/daily-mcp-concurrency-analysis.lock.yml # .github/workflows/smoke-repo-memory-ledger.lock.yml Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Resolved the merge conflicts by merging the latest |
Ran |
|
|
|
🎉 This pull request is included in a new release. Release: |
Redesigns ledger configuration around standalone
tools.ledgerentries, Git branches underledgers/<name>, read-only SQLite projections, and safe-output append requests instead of the legacy repo-memory MCP path.Configuration
tools.repo-memory.ledgerusage.Agent integration
ledger_appendsafe-output capability.Trusted persistence
push_ledger_changesjob boundary and versioned transaction artifact reader.Example:
Run: https://github.com/github/gh-aw/actions/runs/36658333295