Conversation
📝 WalkthroughWalkthroughImage tags, hashes, and agent versions are collected from Docker images during logged runs and task creation, stored in session and task SQLite records, migrated into older databases, tested, and documented. ChangesImage provenance persistence
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant RunOrTask
participant DockerContainer
participant collect_image_metadata
participant SQLiteStore
RunOrTask->>DockerContainer: access image and container attributes
DockerContainer-->>collect_image_metadata: image id, labels, attrs
collect_image_metadata-->>RunOrTask: ImageMetadata
RunOrTask->>SQLiteStore: persist image_tag, image_hash, agent_version
SQLiteStore-->>RunOrTask: created session or task record
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/vibepod/core/image_metadata.py (1)
8-8: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReaching across a module boundary for a private helper.
_parse_image_nameis private tovibepod.core.docker; consider exposing a public alias (e.g.parse_image_name) so this dependency is part of the intended surface and refactors indocker.pydon't silently break provenance collection.🤖 Prompt for 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. In `@src/vibepod/core/image_metadata.py` at line 8, Expose a public `parse_image_name` alias in `vibepod.core.docker` and update `image_metadata.py` to import and use it instead of the private `_parse_image_name` helper, keeping the existing parsing behavior unchanged.src/vibepod/core/session_logger.py (1)
40-52: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate migration helper across the two stores.
This mirrors
TaskStore._migrate_schemainsrc/vibepod/core/tasks.py(Lines 142-153) almost verbatim, only differing in table name androw[1]vsrow["name"]. A shared_add_missing_columns(conn, table, columns)helper would keep both schemas migrating identically.Note: the OpenGrep f-string/SQL-injection hint here is a false positive — column names come from the module-level
_MIGRATION_COLUMNSconstant, and SQLite has no placeholder for identifiers in DDL.🤖 Prompt for 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. In `@src/vibepod/core/session_logger.py` around lines 40 - 52, Extract the duplicated schema migration logic from session logger’s _migrate_schema and TaskStore._migrate_schema into a shared _add_missing_columns(conn, table, columns) helper. Have both stores call it with their table names and migration column definitions, while preserving each store’s existing schema behavior and safe constant-derived column identifiers.Source: Linters/SAST tools
🤖 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 `@docs/configuration.md`:
- Around line 247-249: Update the documentation around the nullable image_tag
and agent_version fields to state that missing metadata is stored as NULL,
rather than describing it as empty. Preserve the existing examples and automatic
migration statement.
---
Nitpick comments:
In `@src/vibepod/core/image_metadata.py`:
- Line 8: Expose a public `parse_image_name` alias in `vibepod.core.docker` and
update `image_metadata.py` to import and use it instead of the private
`_parse_image_name` helper, keeping the existing parsing behavior unchanged.
In `@src/vibepod/core/session_logger.py`:
- Around line 40-52: Extract the duplicated schema migration logic from session
logger’s _migrate_schema and TaskStore._migrate_schema into a shared
_add_missing_columns(conn, table, columns) helper. Have both stores call it with
their table names and migration column definitions, while preserving each
store’s existing schema behavior and safe constant-derived column identifiers.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 228766a4-35d4-46db-82b9-26f909118e34
📒 Files selected for processing (11)
docs/configuration.mdsrc/vibepod/commands/run.pysrc/vibepod/commands/task.pysrc/vibepod/core/image_metadata.pysrc/vibepod/core/session_logger.pysrc/vibepod/core/tasks.pytests/test_image_metadata.pytests/test_run.pytests/test_session_logger.pytests/test_task_cmd.pytests/test_tasks.py
![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: 56783a0683
ℹ️ 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".
| existing = {row[1] for row in conn.execute("PRAGMA table_info(sessions)").fetchall()} | ||
| for column, definition in _MIGRATION_COLUMNS.items(): | ||
| if column not in existing: | ||
| conn.execute(f"ALTER TABLE sessions ADD COLUMN {column} {definition}") |
There was a problem hiding this comment.
When two vp run processes first open the same pre-upgrade logging database concurrently, both can read the old column list before either finishes migrating it, causing the second ALTER TABLE to raise sqlite3.OperationalError: duplicate column name. Because run.py calls open_session after starting the container but before entering its attach/cleanup block, this aborts the command and leaves that container running; serialize the migration or tolerate a column added concurrently. The equivalent migration loop in TaskStore has the same race.
Useful? React with 👍 / 👎.
| containers in tests, or a daemon that dropped away mid-inspect must never | ||
| break the launch — missing pieces simply become ``None``. | ||
| """ | ||
| _, image_tag = _parse_image_name(image) |
There was a problem hiding this comment.
When an agent image override uses a valid bare image ID such as sha256:<hex>, _parse_image_name interprets the colon as a repository/tag separator, so the persisted image_tag becomes only <hex>. This records a nonexistent tag and drops the digest algorithm from the provenance data; detect bare image IDs before applying the repository/tag parser and leave the tag null or preserve the complete digest.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/vibepod/core/image_metadata.py (1)
69-74: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard the
attrsfallback as well.
getattr(container, "attrs", None)can execute a property that raises. Unlike the preceding Docker property reads, this access is unguarded, so a daemon/API failure can break launch despite the function’s best-effort contract.Suggested fix
if image_hash is None: - attrs = getattr(container, "attrs", None) - if isinstance(attrs, dict): - attr_image = attrs.get("Image") - if isinstance(attr_image, str) and attr_image: - image_hash = attr_image + try: + attrs = getattr(container, "attrs", None) + if isinstance(attrs, dict): + attr_image = attrs.get("Image") + if isinstance(attr_image, str) and attr_image: + image_hash = attr_image + except Exception: + pass🤖 Prompt for 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. In `@src/vibepod/core/image_metadata.py` around lines 69 - 74, Guard the `getattr(container, "attrs", None)` access in the image-hash fallback so property or daemon/API errors are caught and do not escape the function. Preserve the existing dictionary and non-empty `"Image"` extraction behavior when the access succeeds, consistent with the best-effort handling of the surrounding Docker property reads.
🤖 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 `@src/vibepod/core/sqlite_migrations.py`:
- Around line 19-25: Update the migration helper around the PRAGMA and ALTER
TABLE statements to validate or allow-list the interpolated table, column, and
definition inputs, rejecting malformed or unsupported values. Since identifiers
cannot use parameter placeholders, quote validated table and column identifiers
safely before constructing SQL, and only permit approved schema type
definitions.
---
Outside diff comments:
In `@src/vibepod/core/image_metadata.py`:
- Around line 69-74: Guard the `getattr(container, "attrs", None)` access in the
image-hash fallback so property or daemon/API errors are caught and do not
escape the function. Preserve the existing dictionary and non-empty `"Image"`
extraction behavior when the access succeeds, consistent with the best-effort
handling of the surrounding Docker property reads.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c56f4b36-c55b-4e45-809c-0bb2eef023ab
📒 Files selected for processing (7)
docs/configuration.mdsrc/vibepod/core/image_metadata.pysrc/vibepod/core/session_logger.pysrc/vibepod/core/sqlite_migrations.pysrc/vibepod/core/tasks.pytests/test_image_metadata.pytests/test_sqlite_migrations.py
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/configuration.md
| existing = {row[1] for row in conn.execute(f"PRAGMA table_info({table})").fetchall()} | ||
| changed = False | ||
| for column, definition in columns.items(): | ||
| if column in existing: | ||
| continue | ||
| try: | ||
| conn.execute(f"ALTER TABLE {table} ADD COLUMN {column} {definition}") |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Validate all interpolated schema inputs.
table, column, and definition are inserted directly into SQL. Current callers pass constants, but this public helper accepts arbitrary strings; a future caller could introduce SQL injection or break migrations with malformed identifiers. Parameter placeholders cannot bind identifiers, so validate/allow-list names and type definitions, then quote identifiers before constructing these statements.
🧰 Tools
🪛 OpenGrep (1.26.0)
[ERROR] 19-19: SQL query built via f-string passed to execute()/executemany(). Use parameterized queries with placeholders instead.
(coderabbit.sql-injection.python-fstring-execute)
[ERROR] 25-25: SQL query built via f-string passed to execute()/executemany(). Use parameterized queries with placeholders instead.
(coderabbit.sql-injection.python-fstring-execute)
🤖 Prompt for 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.
In `@src/vibepod/core/sqlite_migrations.py` around lines 19 - 25, Update the
migration helper around the PRAGMA and ALTER TABLE statements to validate or
allow-list the interpolated table, column, and definition inputs, rejecting
malformed or unsupported values. Since identifiers cannot use parameter
placeholders, quote validated table and column identifiers safely before
constructing SQL, and only permit approved schema type definitions.
Source: Linters/SAST tools
add-trailing-comma and ruff-format landed on main (#126) after this branch was cut; align the branch files so the pre-commit CI check passes.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_task_cmd.py (1)
112-112: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep the headless-agent error message synchronized.
AGENT_SPECS["jcode"]andAGENT_SPECS["tau"]have non-emptyheadless_prefixvalues. However,src/vibepod/commands/task.pystill lists onlyclaude,codex, andauggiein its validation message. Build the message from agents with aheadless_prefix, or update the list.🤖 Prompt for 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. In `@tests/test_task_cmd.py` at line 112, Update the validation error message in the task command to include every agent whose AGENT_SPECS entry has a non-empty headless_prefix, including jcode and tau; preferably derive the listed names dynamically from AGENT_SPECS instead of maintaining a separate hard-coded list.
🤖 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.
Outside diff comments:
In `@tests/test_task_cmd.py`:
- Line 112: Update the validation error message in the task command to include
every agent whose AGENT_SPECS entry has a non-empty headless_prefix, including
jcode and tau; preferably derive the listed names dynamically from AGENT_SPECS
instead of maintaining a separate hard-coded list.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 85cee174-fa74-4239-814f-061dea8cee13
📒 Files selected for processing (13)
docs/configuration.mdsrc/vibepod/commands/run.pysrc/vibepod/commands/task.pysrc/vibepod/core/image_metadata.pysrc/vibepod/core/session_logger.pysrc/vibepod/core/sqlite_migrations.pysrc/vibepod/core/tasks.pytests/test_image_metadata.pytests/test_run.pytests/test_session_logger.pytests/test_sqlite_migrations.pytests/test_task_cmd.pytests/test_tasks.py
🚧 Files skipped from review as they are similar to previous changes (9)
- docs/configuration.md
- tests/test_session_logger.py
- tests/test_image_metadata.py
- src/vibepod/commands/task.py
- tests/test_sqlite_migrations.py
- src/vibepod/core/tasks.py
- src/vibepod/core/session_logger.py
- src/vibepod/commands/run.py
- src/vibepod/core/image_metadata.py
1 participant
Adds best-effort collection and persistence of container image metadata (tag, hash, and agent version) for both session and task records, improving traceability and auditing for VibePod runs. The metadata is now stored in the respective SQLite databases, and the schema is automatically migrated to support these new columns. Comprehensive tests have also been added to ensure robust metadata extraction, even in edge cases or failures.
Image metadata collection and persistence:
ImageMetadatadataclass and acollect_image_metadatafunction insrc/vibepod/core/image_metadata.pyto extractimage_tag,image_hash, andagent_versionfrom container images in a best-effort, failure-tolerant manner.src/vibepod/core/session_logger.py) and task recording (src/vibepod/core/tasks.py) logic to store the collected image metadata in the database. This includes schema migrations for backward compatibility and updates to the relevant data models and insert logic.Integration into agent commands:
vp runandvp task createcommands to collect image metadata at launch and pass it through to logging and task persistence layers.Documentation and testing:
docs/configuration.md) to describe the new session and task record columns and their purpose for traceability.Summary by CodeRabbit
NULL.