Skip to content

feat(agent): add package history store#293

Open
rice-riley wants to merge 1 commit into
mainfrom
agent-go-history-215
Open

feat(agent): add package history store#293
rice-riley wants to merge 1 commit into
mainfrom
agent-go-history-215

Conversation

@rice-riley

@rice-riley rice-riley commented Jun 30, 2026

Copy link
Copy Markdown
Member

Description

This PR adds the Go agent's package-history store. The store gives lifecycle steps a consistent view of the package version requested by the current config and the version most recently recorded on the host, without mutating step definitions or keeping process-local state.

Closes #215

What changed

Version inputs

  • add a Versions value containing the current configured version and the
    previously recorded host version
  • expose those versions as CURRENT_VERSION and PREVIOUS_VERSION
    environment values
  • provide upgrade arguments in [previous, current] order
  • use unknown when no usable prior history exists and uninstalled after a
    completed uninstall check

History persistence

  • store one JSON ledger per package under the configured history directory
  • prepend completed transitions so the newest version remains available as the
    ledger's current version while older entries are preserved
  • normalize recorded timestamps to UTC
  • write history through a same-directory temporary file, sync and close it,
    then atomically replace the ledger
  • create history files with 0600 permissions and history directories with
    0755 permissions

Validation and recovery

  • reject invalid package names, empty package versions, unknown stages, and
    zero timestamps before writing
  • add contextual filesystem errors for failed reads and writes
  • move malformed history to a .backup file and continue from an unknown
    previous version so corrupt host state does not permanently block package
    execution
  • emit structured messages for missing history and corrupt-file recovery

Test coverage

The history suite covers missing and existing ledgers, ordered version updates, uninstall state, corrupt-history recovery, atomic-write cleanup, file permissions, input validation, and filesystem error propagation.

Testing

  • make unit-tests
  • make lint

Checklist

  • I am familiar with the Contributing Guidelines.
  • My commits are signed off (git commit -s) per the DCO.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@rice-riley rice-riley self-assigned this Jun 30, 2026
@rice-riley rice-riley requested a review from a team June 30, 2026 22:45
@github-actions github-actions Bot added component/operator Skyhook operator (controller-manager) component/agent Skyhook agent (package executor) component/ci CI workflows, GitHub Actions, and repo tooling labels Jun 30, 2026
@coderabbitai

coderabbitai Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

Adds a new Go package, history, for per-package install-history persistence. It defines version constants and a Versions type with environment and upgrade-argument helpers, plus a Store type with Path, Read, and Record methods. The implementation loads and updates JSON ledgers, handles missing and corrupted files by recovering or renaming to .backup, and writes atomically through temporary files. A Ginkgo/Gomega test suite covers read, record, recovery, validation, and filesystem-error cases.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

🚥 Pre-merge checks | ✅ 3 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Linked Issues check ⚠️ Warning [#215] The PR covers history storage, but the linked issue also requires flags/log helpers and parity tests that are not shown here. Implement the missing internal/flags and log-path helper work, plus the specified parity tests, or narrow the linked issue scope.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title is concise and accurately summarizes the main change: adding a package history store.
Description check ✅ Passed The description matches the history-store changes and test coverage shown in the summary.
Out of Scope Changes check ✅ Passed The changes stay focused on history-store functionality and tests, with no clear unrelated additions.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch agent-go-history-215

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@agent/go/internal/history/history_test.go`:
- Line 24: The Store logger dependency is using slog directly instead of the
project-standard logr.Logger from log.FromContext(ctx). Update the
Store/NewStore logging API and the related history_test setup to use logr-based
logging rather than constructing a *slog.Logger, and make sure any logger calls
in history.go (including load() and s.logger.Warn) are compatible with that
interface. Keep the implementation aligned with controller-runtime logging
conventions and remove the direct slog dependency from the Store path.

In `@agent/go/internal/history/history.go`:
- Around line 90-92: Several passthroughs in history.go return underlying errors
without adding local context, which violates the error-wrapping guideline.
Update the error returns in Read, load, and the other affected history methods
to use fmt.Errorf with %w and include a function-specific message (for example,
mentioning Read, load, or the relevant operation) instead of returning bare err
or Versions{}, err. Use the existing function names as anchors so each failure
path clearly identifies where it originated.
- Line 26: Replace the `slog` dependency in `Store` with controller-runtime’s
`logr.Logger` obtained from `log.FromContext(ctx)` so the package matches the
project logging convention. Update the `Store` field and constructor/setup in
`history.go` to hold a `logr.Logger` instead of `*slog.Logger`, and adjust every
`s.logger.Info`/`Warn` call in the affected methods to logr’s key/value style
(including any error logging via `Error`). Keep the existing log messages and
context, but remove any `log/slog` imports and ensure no stdlib logging is
introduced.
- Around line 110-120: The Record path in history.go unconditionally prepends to
ledger.Entries, so the ledger grows forever; add a retention cap when updating
the ledger in Record. Use the existing history types/functions like Record,
load, ledger.Entries, and entry to keep only the most recent N entries (or
another clear policy) before saving, while still preserving the current version
update and newest entry at the front.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: 3e6e0892-cfd4-4eff-ac38-184a1f081a51

📥 Commits

Reviewing files that changed from the base of the PR and between d860984 and 2d69731.

📒 Files selected for processing (2)
  • agent/go/internal/history/history.go
  • agent/go/internal/history/history_test.go

Comment thread agent/go/internal/history/history_test.go
Comment thread agent/go/internal/history/history.go
Comment thread agent/go/internal/history/history.go
Comment thread agent/go/internal/history/history.go
@rice-riley rice-riley force-pushed the agent-go-history-215 branch from 2d69731 to ad37b2e Compare June 30, 2026 23:10

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@agent/go/internal/history/history_test.go`:
- Around line 181-191: Add test coverage in the existing invalid-input case to
verify the empty PackageVersion validation in both history.Read and
history.Record. Use the existing store.Record and store.Read paths with
cfg.PackageVersion set to empty, and assert the returned errors match the
package-version validation messages before any write/read proceeds. Keep the new
checks alongside the current invalid stage, zero timestamp, and invalid package
name assertions so the history.go guards are covered.

In `@agent/go/internal/history/history.go`:
- Around line 153-188: Normalize empty or missing current-version values in
Store.load so syntactically valid ledgers still fall back to UnknownVersion.
After json.Unmarshal in load, check the ledger’s CurrentVersion and, if it is
empty, treat it the same as an unknown history by setting it to UnknownVersion
(and ensure Entries remains initialized). Keep the existing handling in load and
Read-compatible behavior aligned so callers like upgrade/ env-var paths see the
unknown sentinel instead of an empty string.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: 43f0f2e1-b8a4-4ed3-813c-48e04f690339

📥 Commits

Reviewing files that changed from the base of the PR and between 2d69731 and ad37b2e.

📒 Files selected for processing (2)
  • agent/go/internal/history/history.go
  • agent/go/internal/history/history_test.go

Comment thread agent/go/internal/history/history_test.go
Comment thread agent/go/internal/history/history.go
Signed-off-by: Riley Rice <riceriley59@gmail.com>
@rice-riley rice-riley force-pushed the agent-go-history-215 branch from ad37b2e to 1b87335 Compare June 30, 2026 23:18

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@agent/go/internal/history/history_test.go`:
- Around line 210-215: The history tests only cover invalid package-name
rejection through Read; update history_test.go to also assert the write path
rejects the same invalid package name before persisting. Add a Record-based
assertion alongside the existing Read checks, using the existing store and cfg
setup in the history test suite, so the behavior is covered via both the Record
and Read flows.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: 8ee4bd63-938a-4c43-bed9-f2ef3e305fa9

📥 Commits

Reviewing files that changed from the base of the PR and between ad37b2e and 1b87335.

📒 Files selected for processing (2)
  • agent/go/internal/history/history.go
  • agent/go/internal/history/history_test.go

Comment thread agent/go/internal/history/history_test.go
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component/agent Skyhook agent (package executor) component/ci CI workflows, GitHub Actions, and repo tooling component/operator Skyhook operator (controller-manager)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[FEA]: Filesystem helpers (paths, flags, log paths) + history file

1 participant