Skip to content

Finish PR #18 Codex fixes for Knowledge Loop #19

Description

@Svaag

Context

Knowledge PR #18 is the Knowledge Loop agent implementation:

  • PR: Add Knowledge Loop agent #18
  • Current head: 82a014fe0804d32f344d73db3090dc3d6d2fb8c6
  • Merge state when captured: CLEAN
  • CI/validate when captured: SUCCESS
  • Local validation passed before the latest Codex findings:
    • uv run ruff check src tests
    • uv run mypy --strict src
    • uv run pytest (86 passed)
    • uv run hyrule-knowledge validate okf
    • uv run hyrule-knowledge quality --check
    • uv run hyrule-knowledge export --check
    • uv run hyrule-knowledge eval --check
    • uv run hyrule-knowledge ledger --check
    • uv run hyrule-knowledge ledger lifecycle --check
    • uv run hyrule-knowledge scan-secrets okf exports reports evals ledger schema

The Codex review cycle is not done yet; no root-post 👍 has been observed.

Remaining Codex findings to fix

1. Keep idle loop runs from publishing timestamp-only refreshes

Thread: #18 (comment)

Codex notes that the unconditional ingest step can rewrite generated concepts/exports with fresh timestamp/run metadata (last_verified_at, manifest run_id, claim timestamps) even when source snapshots are unchanged. _git_dirty() then sees a dirty worktree and --create-pr can open timestamp-only refresh PRs, making the intended idle path unreachable in real loop runs.

Suggested direction:

  • Ensure no-op refresh cycles do not publish timestamp-only diffs.
  • Options may include detecting/reverting timestamp-only generated/export diffs, adding deterministic/no-timestamp ingest mode, or comparing semantic/source-backed output before publish.
  • Add regression coverage proving an unchanged ingest cycle can report idle and does not publish.

2. Resolve learning-event paths against the repo

Thread: #18 (comment)

Codex notes that with --repo-path set and a relative --learning-event, budget preflight currently counts events relative to the supervisor cwd, while ledger import later interprets the same relative path with cwd=repo. This can count a different file, fail before import, or make budget/import behavior inconsistent.

Suggested direction:

  • Resolve relative learning-event paths against config.repo_path before budget preflight and import.
  • Pass the resolved paths consistently to both load_learning_events(...) and ledger import.
  • Add regression coverage for invoking the loop from outside the repo with repo-relative --learning-event.

Already fixed in this review cycle

Codex findings already addressed and resolved on PR #18:

  • Ignore loop state before checking for knowledge changes.
  • Persist provider budgets even when later steps fail.
  • Count learning events, not input files, for import budgets.
  • Charge OpenRouter attempts before child validation can fail.
  • Acquire the loop lock atomically.
  • Create loop PR branches from the base branch.
  • Gate PR budget only for publishing cycles.
  • Reset back to the base branch between loop runs.
  • Charge learning-event budget before imports can partially succeed.
  • Do not evict live lock holders after TTL.
  • Configure a commit identity for the loop.
  • Guard blank loop budget environment defaults.

Done criteria

  • Fix both remaining Codex findings.
  • Add/update regression tests.
  • Run full repository validation.
  • Amend/push PR Add Knowledge Loop agent #18.
  • Resolve fixed Codex threads.
  • Request another @codex review.
  • Poll until either new actionable threads appear or Codex gives 👍 on the PR root post.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type
    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions