Add Knowledge Loop runtime scaffold#302
Conversation
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 52b648ddc3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Address the two Codex findings on the Knowledge Loop runtime scaffold: - Pin the loop checkout. apply.yml always passes knowledge_loop_apply=true for the engineering-loop playbook, so without a host-var override the first apply would deploy whatever AS215932/knowledge `main` points at — non-reproducible and able to pick up unreviewed changes. Pin knowledge_loop_version on `loop` to the merged 0b414ae (knowledge#18, ships `hyrule-knowledge loop --once`) with the timer left disabled. - Document the runner policy refresh. The bootstrap runbook only wrote the knowledge-loop policy, but apply.yml mints the SecretID with the runner's Vault token, which needs the new auth/approle/role/knowledge-loop/* paths from github-runner.hcl first. Add the `vault policy write github-runner` step so the first apply does not fail permission denied. Add an IaC contract test asserting the pin is a 40-char commit and the runbook documents the github-runner policy refresh. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b48083dfbc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex flagged that the wrapper passed --repo-path = the pinned install_dir, so a timer run with --create-pr would branch/commit in the Ansible-managed runtime checkout. A successful run could leave /opt/knowledge-loop on a bot branch or dirty instead of the pinned knowledge_loop_version, and the apply git task uses force: false so it would not reliably repair it. Mirror the engineering_loop split: - Add knowledge_loop_workspace_dir / knowledge_loop_repo_workspace under the state dir and clone the Knowledge repo there at apply time. - Point the loop wrapper --repo-path at the workspace clone; the CLI still runs from the pinned install_dir venv. - Drop install_dir from the service ReadWritePaths so the reviewed runtime stays read-only; only the state dir (workspace + daemon state) is writable. - Validate the new paths and add a contract test for the separation. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 14113828b1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The knowledge_loop role runs before the knowledge-loop vault_agent renders /etc/knowledge-loop/knowledge-loop.env and the GitHub App key. With the timer's Persistent=true, enabling it after the scheduled time fires the service immediately on activation, so a canary apply that both enables the timer and first renders secrets could trigger a run before the EnvironmentFile/key exist. Gate the timer start on the rendered env file and private key existing: stat both and only set state=started when present, otherwise enable the unit and let the next apply (post-render) start it. Add a contract test for the guard. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1c37633eaf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…handler Two ordering/handler findings from Codex: - Per-instance Vault Agent restart handler (P1). engineering-loop.yml includes the vault_agent role twice (engineering-loop + knowledge-loop). Ansible handlers are a single play-level namespace, so the shared `restart vault agent` handler name was deduped and a notify could restart the wrong instance, leaving the other on stale config/secrets. Template the handler name and its notifies with vault_agent_name so each instance gets its own handler. - Keep the secrets gate when restarting the timer (P2). The restart knowledge-loop timer handler bypassed the guard on the Set Knowledge Loop timer state task, so a changed checkout/template could restart the Persistent=true timer and fire the service before the env file/key exist. Apply the same knowledge_loop_secret_files existence guard in the handler. Extend the IaC contract tests for both. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9da6cf1564
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Two more Codex findings: - Static/in-role Vault Agent restart. The previous templated handler name is still unreliable: Ansible resolves handler names at play load, so two vault_agent includes (engineering-loop + knowledge-loop) cannot each get their own handler. Replace the handler with an in-role restart: register the config/secret/unit tasks and have the existing "Enable and start" step restart when any changed, so vault_agent_name binds at task-execution time and the right instance restarts. - Let Vault-rendered OpenRouter budget take effect. The wrapper always passed --max-openrouter-calls-per-day (Ansible default 0), which overrode the argparse default sourced from HYRULE_KNOWLEDGE_LOOP_MAX_OPENROUTER_CALLS_PER_DAY, so a higher budget in kv/knowledge-loop was ignored and live enrichment was rejected. Only pass the flag when that env var is unset. Update the IaC contract tests for both. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary
knowledge_looprole for the Knowledge Loop producer agentkv/knowledge-loopvault-agent-knowledge-loop.serviceknowledge_loop_apply=trueand mint the Knowledge Loop AppRole bootstrapSafety
0Validation
python -m pytest tests/iac/test_vault_and_runner_contracts.py -qscripts/ci/render-all.shscripts/ci/deploy-preflight.sh --repo-onlyansible-playbook playbooks/engineering-loop.yml --tags validate --connection=local --limit loop --skip-tags snapshotDependency
Merge/deploy after
AS215932/knowledge#18so a follow-up deploy PR can pinknowledge_loop_versionto a commit that containshyrule-knowledge loop --once.