Consolidate ledger compaction into two Agentic Maintenance jobs - #64709
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft.
|
|
✅ Ponytail Reviewer completed successfully! 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. Unable to emit PR review writes from this environment after safeoutputs write attempts were denied at the shell boundary while reviewing PR #64709.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Shared failure and concurrency behavior can skip valid plans, while case-sensitive ledger names can collide on supported runners.
Review effort: Balanced
Findings: 2
Open (2)
What changed in this PR
Consolidates per-ledger compaction into one planning job and one trusted application job.
Changes:
- Generates per-ledger steps and plan files within two shared jobs.
- Updates tests, documentation, and generated workflow YAML.
- Preserves read/write permission separation and plan validation.
| File | Description |
|---|---|
pkg/workflow/maintenance_workflow_ledger_compaction.go |
Generates consolidated compaction jobs. |
pkg/workflow/maintenance_workflow_ledger_compaction_test.go |
Tests the two-job structure. |
docs/src/content/docs/experimental/ledger-compaction.md |
Documents consolidated execution. |
.github/workflows/agentics-maintenance.yml |
Applies the generated workflow changes. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| group: gh-aw-ledger-compaction-${{ github.repository }}-` + ledger.Name + ` | ||
| cancel-in-progress: false | ||
| `, | ||
| planFile := path.Join(maintenanceLedgerCompactionPlanDir, "plan-"+ledger.Name+".json") |
| concurrency: | ||
| group: gh-aw-ledger-compaction-${{ github.repository }} | ||
| cancel-in-progress: false |
🏗️ ADR Required - draft added for PR #64709I added a draft ADR at Evidence used
Inferred decisionConsolidate ledger compaction orchestration into two repository-wide maintenance jobs while preserving the trust boundary and per-ledger validation through step-level execution and per-ledger plan files. Next actionPlease review and refine the draft ADR, then keep it with this implementation so the architectural change is documented explicitly before merge.
|
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.
Skills-Based Review 🧠
Applied /codebase-design and /grill-with-docs — this is a clean architectural consolidation (6 job pairs → 2 jobs, per-step conditions instead of per-job), with good test coverage of the new structure and no change to the validated trust boundary (plan job stays read-only/untrusted, apply job stays write/trusted with expected-head-guard revalidation). Submitting as COMMENT since the issues raised are about fault-isolation trade-offs worth a deliberate decision, not correctness bugs.
📋 Key Themes & Highlights
Key Themes
- Fault isolation changed: consolidating per-ledger jobs into shared jobs means one slow/hanging selection script or one rejected/failed apply for a ledger can now affect sibling ledgers' steps in the same job run — this wasn't true before. Worth either mitigating (timeouts,
continue-on-erroron apply steps) or explicitly documenting as accepted behavior. - Docs: the trust-boundary table nicely reflects the job consolidation but doesn't call out the fault-isolation trade-off.
Positive Highlights
- ✅ Test coverage was properly updated (
TestGenerateMaintenanceWorkflow_LedgerCompaction, case-sensitivity test) to assert the new step-ID-based structure, not just job-count changes. - ✅ Per-ledger plan files (
plan-<name>.json) in a shared dir avoid the earlier singleplan.jsoncollision risk and keep trust/validation logic (validatePlan, expected-head guard) unchanged per ledger. - ✅ Concurrency group simplification to repository-wide (
gh-aw-ledger-compaction-${{ github.repository }}) is consistent with the new single-job model.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 117.1 AIC · ⌖ 14.6 AIC · ⊞ 10K
Comment /matt to run again
| id: plan | ||
| uses: ` + job.githubScriptPin + ` | ||
| for _, job := range jobs { | ||
| b.WriteString(` - name: Plan ledger compaction (` + job.ledger + `, untrusted, read-only) |
There was a problem hiding this comment.
[/codebase-design] Consolidating 6 untrusted plan steps into one job means a single misbehaving/slow selection script (e.g. an infinite loop or hang in one ledger's compaction.script) can block or exhaust the time budget for every other ledger's plan step in the same job run, whereas the previous per-ledger job design isolated that fault.
💡 Suggestion
Consider adding a per-step timeout-minutes (the ledger_compaction.cjs worker already enforces MAX_WORKER_INPUT_BYTES/timeout internally for the script sandbox, but the outer github-script step itself has no bound) so one slow ledger can't starve the others sharing the job, and so the shared job's overall runtime stays predictable as more ledgers are added.
@copilot please address this.
| if: ${{ steps.plan.outputs.plan_created == 'true' }} | ||
| `) | ||
| } | ||
| b.WriteString(` - name: Upload ledger compaction plans |
There was a problem hiding this comment.
[/codebase-design] The single ledger-compaction-plans artifact and single ledger_compaction_apply job mean one ledger's plan being rejected/invalid during apply raises RejectedPlanError via validatePlan/prepareApply per-step, but since each apply step is independent (if: needs...stepID_created) a thrown error in one apply step will still fail the whole job and skip remaining ledger applies for that run (GitHub Actions stops subsequent steps by default unless continue-on-error/if: always() is set).
💡 Suggestion
Add continue-on-error: true to each per-ledger apply step (mirroring the pattern used elsewhere in this codebase, e.g. checkout_step_generator.go's credential-cleanup step), or restructure so a rejected plan for ledger A doesn't prevent ledger B's plan from being applied in the same run. Confirm the intended behavior and document it either way, since this is a change in fault-isolation semantics from the prior per-ledger job design.
@copilot please address this.
| concurrency group: | ||
| All compaction-enabled ledgers share two maintenance jobs and a repository-wide | ||
| compaction concurrency group: | ||
|
|
There was a problem hiding this comment.
[/grill-with-docs] The trust-boundary table now describes two shared jobs, but doesn't mention the fault-isolation trade-off introduced by consolidation (a hung/slow selection script or an apply failure for one ledger can affect the shared job run for all ledgers). Given this is a security-relevant trust-boundary doc, it's worth being explicit about what changed in the failure/isolation model, not just the job count.
💡 Suggestion
Add a short sentence noting that ledgers now share job-level fault domains (one slow/failing step can block sibling steps in the same job run), so operators know this is a deliberate trade-off for fewer jobs, not an oversight.
@copilot please address this.
There was a problem hiding this comment.
Reviewed with the impeccable harden / audit lens (refactor/cleanup of shared-infra jobs) focused on correctness, reliability, and edge-state handling of the new consolidated plan/apply jobs.
Findings
| # | Severity | File | Lines | Issue | Confidence |
|---|---|---|---|---|---|
| 1 | 🟠 HIGH | pkg/workflow/maintenance_workflow_ledger_compaction.go |
165-179 | Per-ledger plan steps now run sequentially inside one shared job with no continue-on-error/if: always(); a single ledger's step throwing (script re-throws on unexpected errors) aborts the job and skips planning/upload/apply for every other ledger in the run, a regression from the prior per-ledger job isolation |
8/10 |
Other observations (non-blocking)
- The per-ledger
applysteps (lines 213-228) have the same sequential-failure characteristic on the trusted side; consider the same guard there for consistency once the plan-side fix lands. - Docs (
docs/src/content/docs/experimental/ledger-compaction.md) were updated consistently with the new two-job model — good. - Tests were updated to match the new job/step shape and assert the shared concurrency group and consolidated artifact name — good coverage of the structural change, but no test currently exercises one ledger's step failing while others succeed.
Requesting changes mainly for item #1, since it silently degrades the ledger-compaction guarantee (every enabled ledger gets a chance to run each maintenance cycle) that the single-job-per-ledger design previously provided.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 119.3 AIC · ⌖ 13.1 AIC · ⊞ 8.1K
| @@ -173,37 +176,45 @@ func writeLedgerCompactionPlanJob(b *strings.Builder, opts buildMaintenanceWorkf | |||
| const { main } = require('${{ runner.temp }}/gh-aw/actions/ledger_compaction_plan.cjs'); | |||
| await main(); | |||
There was a problem hiding this comment.
Missing failure isolation between ledgers sharing this job.
Previously each ledger had its own ledger_compaction_plan_<name> job, so one ledger's failure could not affect any other ledger. Now all ledgers' plan steps run sequentially inside a single ledger_compaction_plan job with no continue-on-error: true / if: always() on each step.
ledger_compaction_plan.cjs.main() re-throws on any unexpected error (see its catch block), which fails the actions/github-script step. By default GitHub Actions skips all subsequent steps in a job once one fails, so a single misbehaving or misconfigured ledger (bad selection script, network error, malformed config) silently prevents planning (and therefore the upload + apply) for every ledger ordered after it in the same run.
Consider adding continue-on-error: true to each per-ledger step (and aggregating failures explicitly), or wrapping the await main() calls so a single ledger's exception cannot abort the whole job.
@copilot please address this.
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Implemented the ledger compaction fixes in |
|
@copilot do not use base64 to encode config. Must be human readable. |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Updated ledger compaction configuration to use indented JSON in the workflow environment instead of base64, with the parser and tests updated accordingly. Commit: |
|
🎉 This pull request is included in a new release. Release: |
Agentic Maintenance currently creates a plan job and an apply job for every ledger. This change consolidates them into one plan job and one apply job.