Make entries.updated nullable — NULL on insert, set only on update#92
Conversation
The updated column was previously set to now() on initial insert, making it identical to created. This is semantically wrong: a new entry hasn't been updated yet. Aligns with the users.updated fix from PR #91. - Schema: Entry.updated is now Optional[datetime], age_sec falls back to created - DDL: entries.updated drops NOT NULL constraint - Store: all INSERT paths pass None; UPDATE paths still set now() - Sort/filter: COALESCE(updated, created) throughout (ORDER BY, since/until) - Migration: backfills NULL where updated = created - 455 tests pass, ruff clean, mypy clean Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
cmeans
left a comment
There was a problem hiding this comment.
QA Review — PR #92: Make entries.updated nullable
Summary
Clean semantic fix — `entries.updated` is now NULL on insert, set only on actual updates. Mirrors the `users.updated` fix from PR #91. All queries use `COALESCE(updated, created)` for sort/filter consistency.
Code Review
Migration (`h3c4d5e6f7g8`)
- Drops NOT NULL on `entries.updated`
- Backfills: `UPDATE entries SET updated = NULL WHERE updated = created` — correct for existing data
- Clean downgrade: restores from COALESCE + re-adds NOT NULL
- Revision chain correct: depends on `g2b3c4d5e6f7`
Schema (`schema.py`)
- `updated: datetime | None = None` — nullable with default
- `to_dict()` and `to_list_dict()`: `to_iso(self.updated) if self.updated else None`
- `from_dict()`: uses `ensure_dt_optional` for updated
- `age_sec`: `ref = self.updated or self.created` — correct fallback
DDL (`create_tables.sql`)
- `updated TIMESTAMPTZ` — nullable, no default
PostgresStore
- Default ORDER BY: `COALESCE(updated, created) DESC` in `_query_entries()`
- All `since`/`until` filters: `COALESCE(updated, created) >= %s`
- `updated=now` removed from all Entry constructors (upsert_status, upsert_alert, upsert_preference)
- SQL files updated: `get_entries_without_embeddings.sql`, `get_stale_embeddings.sql`
Tools (`tools.py`)
- `updated=now` removed from learn_pattern, remember, suppress_alert, add_context, remind
- `update_entry` response: `to_iso(result.updated) if result.updated else None`
Collator/Prompts
- `last_report`: `to_iso(status.updated or status.created)` — correct fallback for display
Server
- `_do_embed` dummy Entry: `updated=None`
Observations
- `seed-demo.sql` still explicitly sets `updated` in INSERT column lists. New seed entries will have `updated = created` rather than NULL. Not a blocker — seed data predates this change — but could be aligned in a follow-up.
CI Gate Check
| Check | Conclusion |
|---|---|
| lint | ✅ SUCCESS |
| typecheck | ✅ SUCCESS |
| test (3.10) | ✅ SUCCESS |
| test (3.11) | ✅ SUCCESS |
| test (3.12) | ✅ SUCCESS |
| codecov/patch | ✅ SUCCESS |
Zero non-SUCCESS/non-SKIPPED checks.
Test Results
| Check | Result |
|---|---|
| pytest (455 tests) | ✅ 455/455 pass |
| ruff (src/, alembic/) | ✅ Clean |
| mypy | ✅ Clean |
| CI | ✅ All green |
| Manual #1: New entry has null updated | ✅ SQL: `updated IS NULL = true` |
| Manual #2: update_entry sets timestamp | ✅ `updated` set to non-null after update, MCP returns timestamp |
| Manual #3: Updated entry shows in knowledge | ✅ `has_updated = true`, description changed |
| Manual #4: Status staleness detection | ✅ `age_sec` uses `COALESCE(updated, created)`, TTL-expired entry detected |
| Manual #5: Since filter with COALESCE | ✅ Never-updated entries included in since-filtered queries |
| Manual #6: Sort order with mixed NULL/non-NULL | ✅ `COALESCE(updated, created) DESC` sorts correctly |
Verdict
Zero findings (1 observation about seed-demo.sql). All CI green. All manual tests pass. Ready for QA Signoff.
|
Adding Ready for QA Signoff — entries.updated nullable change verified across migration, schema, DDL, all queries (COALESCE), tools, collator, prompts. CI fully green (codecov SUCCESS), 455/455 pytest, 6/6 manual tests. One observation: seed-demo.sql still sets updated explicitly. |
Addresses QA round-1 finding #1 on PR #333. The actually-merged mcp-clipboard PRs are: - #88 (merged 2026-04-20T01:31:47Z) — original env-routing hardening. #87 does not exist in that repo. - #92 (merged 2026-04-20T20:26:12Z) — comment-escape cascade. #90 was superseded and closed without merging. No code change; CHANGELOG narrative only. PR body updated in parallel with the same corrections (including the pull_request / pull_request_target trigger correction for pr-labels.yml). Symmetric hardening follow-up for pr-labels.yml filed as #334 (P3, non-blocking). Refs #332. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…on (#332) (#333) Closes #332. ## Summary `.github/workflows/pr-labels-ci.yml` on this repo was in the pre-`cmeans/mcp-clipboard#88` state for the sibling `cmeans/*` cascade: contributor-controlled `workflow_run` fields (`head_branch`, `id`, `repository`) were inlined directly as `${{ ... }}` expressions inside `run:` bodies. Git refnames allow shell metacharacters (`$`, backtick, `;`, `&`, `|`, etc.), so a malicious fork-PR branch name would render as directly-executed shell once GHA substituted the expression at queue time. This PR cascades the hardening from `cmeans/mcp-clipboard`: 1. **#88 pattern — env routing.** All contributor-controlled values now flow through step-level `env:` blocks and are referenced as shell variables (`"$HEAD_BRANCH"`, `"$REPO"`, `"$RUN_ID"`). Job-level `if:` conditionals continue to use `${{ ... }}` (that expression context is safe — it's evaluated by GHA itself, not handed to a shell). 2. **#92 pattern — comment escape.** Comments inside `run:` blocks avoid any literal `${{ }}` sequence. GHA's queue-time parser substitutes expressions even inside shell `#` comments and rejects empty expressions with `An expression was expected` on `workflow_dispatch` / fresh-repo registration. Without this, the #88 hardening would have landed a latent trap. Cascaded verbatim from `cmeans/mcp-clipboard`'s `.github/workflows/pr-labels-ci.yml` at the `main` HEAD after PR #92 merged (2026-04-20T20:26:12Z), preserving this repo's AGPL-3.0 header at lines 1–16. No functional behavior change — label transitions, PR lookup, force-push tolerance, and Dev Active skip logic are all byte-identical to before, just with shell values now arriving via environment instead of via text substitution. ## Scope - `.github/workflows/pr-labels-ci.yml` — hardened (22 lines changed) - `CHANGELOG.md` — `[Unreleased] → ### Security` entry No source, tests, migrations, or docs. ## References - Root diagnosis of the GHA parser quirk: [cmeans/yt-dont-recommend#28](cmeans/yt-dont-recommend#28) - Original security hardening (merged): [cmeans/mcp-clipboard#88](cmeans/mcp-clipboard#88) - Comment-escape cascade source (merged): [cmeans/mcp-clipboard#92](cmeans/mcp-clipboard#92) - Follow-up for symmetric `pr-labels.yml` hardening (non-blocking, P3): [#334](#334) ## QA ### Prerequisites Pure workflow-file change. No deploy, no env setup, no Python changes. All verification is static (`yq`/`python -c`, `grep`) plus an optional post-merge Actions UI smoke. ### Automated checks The CHANGELOG check, ruff, mypy, pytest, Codecov — none of these touch the workflow file. They should all pass unchanged against `main`. Confirm via the green CI checks on this PR before approving. ### Manual tests All local verification runs from the repo root on the PR branch (`fix/pr-labels-ci-hardening-332`). 1. - [x] **YAML parse is clean.** ``` python3 -c "import yaml; yaml.safe_load(open('.github/workflows/pr-labels-ci.yml')); print('OK')" ``` Expected: prints `OK`, exit 0. Confirms no syntax regression. 2. - [x] **No literal empty `${{ }}` anywhere in the file.** ``` grep -n '\${{ }}' .github/workflows/pr-labels-ci.yml || echo "(none — good)" ``` Expected: prints `(none — good)`. Guarantees the `#92` parser trap can't surface on `workflow_dispatch` / fresh-repo registration. 3. - [x] **No contributor-controlled field appears inside a `run:` body as an expression.** ``` awk '/^ run: \|/,/^ - name:|^ [^ ]|^jobs:/' .github/workflows/pr-labels-ci.yml | grep -nE '\$\{\{ *github\.event\.workflow_run\.(head_branch|id) *\}\}' || echo "(none — good)" ``` Expected: prints `(none — good)`. The only remaining `${{ github.event.workflow_run.head_branch }}` references are inside job-level `if:` conditionals (safe context, evaluated by GHA not a shell) and step-level `env:` assignments (safe by design — the whole point of the cascade). 4. - [x] **`env:` blocks route the three contributor-controlled fields.** ``` grep -nE '^[[:space:]]+(REPO|RUN_ID|HEAD_BRANCH):' .github/workflows/pr-labels-ci.yml ``` Expected: six lines total (three per job), each assigning from the matching `${{ github.event.workflow_run.* }}` / `${{ github.repository }}` expression. 5. - [x] **`workflows: [CI]` still matches the CI workflow name in this repo.** ``` grep -A1 'workflows:' .github/workflows/pr-labels-ci.yml | head -3; grep '^name:' .github/workflows/ci.yml ``` Expected: `workflows: [CI]` appears in this file AND `name: CI` appears in `.github/workflows/ci.yml`. Names match, so `workflow_run` will still fire on CI completion. 6. - [x] **AGPL header preserved.** ``` sed -n '1,16p' .github/workflows/pr-labels-ci.yml ``` Expected: lines 1–16 are the existing `mcp-awareness — ambient system awareness for AI agents` / `Copyright (C) 2026 Chris Means` / AGPLv3 preamble. Unchanged from `main`. 7. - [x] **Diff against `main` is hardening-only (no behavior drift).** ``` git diff origin/main -- .github/workflows/pr-labels-ci.yml ``` Expected: only adds `REPO:`, `RUN_ID:`, `HEAD_BRANCH:` to the two `env:` blocks, deletes the shell-level `REPO=${{...}}` / `HEAD_BRANCH=${{...}}` assignments, rewrites `API_OUT=...${{ ... }}/pull_requests` to `.../$RUN_ID/pull_requests`, and updates two comment blocks to avoid `${{ }}` literals. No changes to `if:` conditionals, `permissions`, `on:`, job names, label manipulation logic, or `exit 0` paths. 8. - [ ] **(Post-merge) Smoke the label automation end-to-end.** This workflow is `workflow_run`-triggered (no `workflow_dispatch`), so pre-merge dispatch isn't available. After merge, take any in-flight `Awaiting CI` PR and confirm the label transitions still happen on the next CI completion: - Watch `.github/workflows/pr-labels-ci.yml` runs at https://github.com/cmeans/mcp-awareness/actions/workflows/pr-labels-ci.yml - Expected on CI pass: `on-ci-pass` succeeds (~5 s), target PR moves `Awaiting CI → Ready for QA`. - Expected on CI fail: `on-ci-fail` succeeds, target PR gains `CI Failed`. ### Out of scope (explicitly) - **`pr-labels.yml`** (the `on: pull_request:` sibling) is not in this PR. Its current trigger makes `secrets.GITHUB_TOKEN` read-only for fork PRs and the values it does inline into `run:` blocks today are all safe types (numeric, repo name, hex SHA), so no injection vector exists at the current configuration. Symmetric env-routing as defense-in-depth tracked in [#334](#334) (P3, non-blocking). - **Adding `workflow_dispatch` to `pr-labels-ci.yml`** for post-merge verifiability. Not cascaded because mcp-clipboard doesn't have it either. Track separately if desired. --------- Co-authored-by: cmeans-claude-dev[bot] <3223881+cmeans-claude-dev[bot]@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
entries.updatedis now NULL on insert and only set when an entry is actually updated — same semantic fix applied tousers.updatedin PR Add JWT auth middleware, RLS policies, and CLI commands #91COALESCE(updated, created)so never-updated entries sort/filter by creation timeupdated = created(entries that were never truly updated)QA
Prerequisites
pip install -e ".[dev]"AWARENESS_PORT=8421)Manual tests (via MCP tools)
Then:
Expected: entry returned with
"updated": nullUsing the entry ID from step 1:
Expected: response includes
"updated": "<timestamp>", not nullExpected: entry now has
"updated": "<timestamp>"(non-null)Wait 10 seconds, then:
Expected: briefing shows
qa-testas stale with alast_reporttimestampExpected: entries from steps 1-3 appear (COALESCE ensures never-updated entries aren't excluded)
Create a second entry:
Then:
Expected: most recently created/updated entries appear first; never-updated entries sort by created time
🤖 Generated with Claude Code