[omp-harness]

Restructured per owner review

loft-review-surface was a project-specific skill — against the rule that skills here hold reusable procedures only. This PR now:

1. Documents the rule in docs/writing-documentation.md ("Project-specific content lives in the project").
2. Adds test-session-auth (generic): forge a session cookie from the server's own secret (runnable env-parameterized example) or drive the real OAuth flow (skill://google-auth), then verify the rendered page. Carries the invariants that cost sessions: source .env first, identity from env never literals, cookie scoped to the target URL, never document.cookie.
3. Deletes loft-review-surface — the loft-specific surface content (server start via the loft's existing make serve, batch/page URL format, layout-box checks) moves to the loft repo's own docs (separate PR there).

Deployment

make install + omp restart for the new skill. make test green. The lucidlint pin carried earlier is now owned by #56 (merged main 2518f2b); this PR no longer touches that file.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 02b8150)

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 57 - Partially compliant Compliant requirements: The PR updates the lucidlint install pin to f8cbbc8 (0.4.0) in the skill. The pin now matches the 0.4.0 release used in houses. No other skill text changes were made (only the pin was updated). Non-compliant requirements: None. Requires further human verification: None.

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Possible Issue The script inserts the current working directory into sys.path and imports tools.auth. If the user runs the script from a different directory (e.g., from the omp-config repo root), the import will fail with a confusing error. The script's docstring instructs running from the loft repo root, but the error message only says "run from the loft repo root" without checking the actual path. This could lead to a misleading failure if the user is in the wrong directory. sys.path.insert(0, ".") try: from tools.auth import session_secret except ImportError: sys.exit("tools/auth.py not found — run from the loft repo root: this script imports the loft repo's auth module.") Possible Issue The script uses page.setCookie with a url parameter, but the comment says the cookie is scoped to url and applies to any host. However, Puppeteer's setCookie with a url parameter sets the cookie for that exact URL, including the host. If the page is opened on a different host (e.g., localhost vs 127.0.0.1), the cookie may not be sent, causing the session to fail. The comment claims it applies to any host, which may be incorrect. async ({ page }, { cookie, url }) => { await page.setCookie({ name: "session", value: cookie, url }); await page.goto(url, { waitUntil: "networkidle2" });

[github-actions]

Persistent review updated to latest commit 0117402

[github-actions]

Persistent review updated to latest commit 97c7868

[github-actions]

Persistent review updated to latest commit 93b2b61

[github-actions]

Persistent review updated to latest commit 5a3fb36

[github-actions]

Persistent review updated to latest commit 97b0b5b

[github-actions]

Persistent review updated to latest commit 02b8150

[github-actions]

Standalone PR Review

PR-Agent could not safely update the persistent review. This standalone result will not replace the canonical review.

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 56 - Partially compliant Compliant requirements: No requirements from this ticket are addressed by the PR diff. Non-compliant requirements: The diff does not replace skills/code-discovery/ with skills/code-review-graph/. The diff does not introduce the active-use moment table or the branch-switch recovery doctrine. No changes to code-review-graph or code-discovery skills are present. Requires further human verification: This PR appears unrelated to ticket skills: code-review-graph — one skill for discovery, active use, and branch-switch convergence #56: the diff is about a generic test-session-auth skill and the project-specific-skill documentation rule. Verify whether the ticket is correctly linked before treating these as non-compliance findings.

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ No major issues detected

[github-actions]

Standalone PR Review

PR-Agent could not safely update the persistent review. This standalone result will not replace the canonical review.

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis ✅ 56 - PR Code Verified Requires further human verification: This PR's diff does not touch skills/code-discovery/, skills/code-review-graph/, the watcher, or its CLI; it changes test-session-auth and docs only. The PR description states skills: code-review-graph — one skill for discovery, active use, and branch-switch convergence #56's work is already merged into main (2518f2b), so compliance of skills: code-review-graph — one skill for discovery, active use, and branch-switch convergence #56 cannot be assessed from this PR alone. Verify through GitHub that skills: code-review-graph — one skill for discovery, active use, and branch-switch convergence #56 was satisfied by its own merged change.

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Cookie forge may mismatch server serializer The sample recreates the serializer from only secret/salt/payload, leaving key derivation, digest method, and serializer class at itsdangerous defaults. Many apps on the same 'itsdangerous timed-serializer shape' configure non-default values (e.g., Flask's default session cookie uses salt cookie-session, HMAC/SHA1 key derivation, and a tagged JSON serializer). For those apps, a cookie minted with the correct secret and salt is silently rejected (authenticated: false with no error) — the exact silent failure the skill warns about. The sample and skill should tell the agent to read the app's own session-serializer configuration or use its public serializer whenever any non-default parameter is present. salt = os.environ.get("SESSION_COOKIE_SALT") serializer = URLSafeTimedSerializer(secret, salt=salt) if salt else URLSafeTimedSerializer(secret)

[github-actions]

Standalone PR Review

PR-Agent could not safely update the persistent review. This standalone result will not replace the canonical review.

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 56 - Partially compliant Compliant requirements: (none assessable — see requires_further_human_verification) Non-compliant requirements: (none assessable — see requires_further_human_verification) Requires further human verification: Ticket skills: code-review-graph — one skill for discovery, active use, and branch-switch convergence #56's scope (code-review-graph skill consolidation) is not present in this diff; the diff covers the project-specific-skills rule, the generic test-session-auth skill, and removal of loft-specific content. The PR description states skills: code-review-graph — one skill for discovery, active use, and branch-switch convergence #56 is already merged (2518f2b), so the ticket-to-PR association appears mismatched. Human confirmation is needed of which ticket this PR should be checked against before any ticket requirement can be marked compliant.

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Possible Issue The env-read guard only catches KeyError, but the same line calls json.loads on SESSION_COOKIE_PAYLOAD, which raises an unhandled JSONDecodeError when the variable contains malformed JSON. The script then dies with a raw traceback instead of the intended usage message. Trigger: set SESSION_COOKIE_PAYLOAD to a non-JSON value and run the script. try: payload = json.loads(os.environ["SESSION_COOKIE_PAYLOAD"]) secret = os.environ["SESSION_COOKIE_SECRET"] except KeyError as e: sys.exit(f"{e.args[0]} is required — set SESSION_COOKIE_SECRET and SESSION_COOKIE_PAYLOAD " "in the app's env and source it before running") Compliance The new "Project-specific content lives in the project" rule violates the repo's own reviewable-rule form (PRD R2): it states the anti-pattern and gives a heuristic, but never classifies a project-specific skill as a review finding, and names no enforcement mechanism. Sibling rules in the same doc use explicit "is a finding" language. Add an explicit finding class (e.g., "a skill whose core procedure depends on one project's endpoints or commands is a finding") so the review bot can enforce it. ## Project-specific content lives in the project This repo's skills hold **reusable procedures** — a task an agent performs in any house repo. Content that applies to one project only (that repo's endpoints, batch IDs, layout details, app-specific commands) belongs in that project's own docs, linked from there — never in a skill here. A skill written for one app is a project-specific skill: split the generic procedure (which stays here, as its own skill) from the project specifics (which move to the project's repo). Compliance No violations. The new skill keeps secrets in the environment (access/auth constraint, R8), is a reusable procedure rather than project-specific content, and the PR description states make test green and calls out make install + omp restart as required for effect. Compliance No violations. This PR adds no documentation files, so the required doc set and AGENTS.md discoverability rules are unaffected, and skills remain exempt from the reachability rule.

[github-actions]

Standalone PR Review

PR-Agent could not safely update the persistent review. This standalone result will not replace the canonical review.

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis ❌ 56 - Not compliant Non-compliant requirements: The diff does not add skills/code-review-graph/SKILL.md. The diff does not remove or replace skills/code-discovery/SKILL.md. No code-review-graph discovery, active-use, or branch-switch doctrine content is present in the diff. No implementation of the code-review-graph status/build/watcher checks is present. Requires further human verification: The supplied PR branch and diff target the test-session-auth skill, not ticket skills: code-review-graph — one skill for discovery, active use, and branch-switch convergence #56; confirm whether ticket skills: code-review-graph — one skill for discovery, active use, and branch-switch convergence #56 is the correct ticket for this PR.

⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ No major issues detected

[github-actions]

PR Code Suggestions ✨

No code suggestions found for the PR.

[github-actions]

Standalone PR Review

PR-Agent could not safely update the persistent review. This standalone result will not replace the canonical review.

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 56 - Partially compliant Compliant requirements: (None of the ticket requirements are touched by this PR's diffs) Non-compliant requirements: Replace skills/code-discovery/ with a single skills/code-review-graph/ skill Skill must cover discovery precedence, active-use moments, branch-switch doctrine Discovery precedence: structural questions start with graph, not grep Active use: table of common development moments mapped to graph tools Branch-switch doctrine: watcher convergence, silent-stale detection, recovery via incremental build Skill must be alwaysApply and named after the tool code-review-graph No other file should reference skill://code-discovery Deployment: make install + omp restart make test must stay green Requires further human verification: (empty — no ticket requirements are addressed by this PR)

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Missing deploy note The skill is new but SKILL.md does not mention that it takes effect via make install + omp restart. Every skill change in this repo should state that deployment step (as done in PR --- name: test-session-auth description: Sign into a local app as a test user without a human — forge a session cookie from the server's own secret, or drive the real OAuth flow; never hand the login to the user. --- # Test-session auth — sign in without a human ## The rule Never ask the user to log in, and never claim a session-dependent feature works without exercising it under a real session. Sign in yourself. ## Forge the session cookie (fastest) The server's own serializer plus its secret produce a cookie the server accepts as a real session — no browser dance, no provider round-trip. For apps on the itsdangerous timed-serializer shape, run the sample: [forge_session_cookie.py](skill://test-session-auth/examples/forge_session_cookie.py). Invariants (each cost a full session when missed): - **Source the app's env BEFORE forging** — a different secret silently produces an invalid cookie (`authenticated: false` and no error). - **The identity payload comes from the environment**, never literals — no real person's email/name in committed files. - **Set the cookie scoped to the target URL** (`page.setCookie({name, value, url})`) — never `document.cookie` (the harness `run` action executes in Node, not the page DOM) and never a bare `domain: "localhost"` when the page may open on a LAN address. - **Non-itsdangerous serializer**: use the app's own public serializer/session helper — the forge pattern stays the same. ## Drive the real OAuth flow (when the forge cannot cover it) Multi-factor, provider-side state, or a server that verifies against the identity provider: run the real sign-in flow — see `skill://google-auth` for the authorization-code web flow. ## Then verify the rendered page A session is the means; the end is looking at the rendered surface. Screenshot the page and inspect it before claiming the change works — unit tests and evals test data, not the rendered UI. Never delegate the visual check to the user. Missing deploy note on new example The new examples/forge_session_cookie.py file does not state that make install + omp restart is needed for the skill to take effect. The PR description says "make install + omp restart for the new skill" but this is not documented in the skill itself or in AGENTS.md. """Forge a session cookie for an app on the itsdangerous timed-serializer shape (secret + salt + payload). Run after sourcing the app's env, so the secret is the one the server uses: . .env && python <path-to-this-file> > cookie.txt (no trailing newline is written — read cookie.txt raw) Env contract: - SESSION_COOKIE_SECRET — the server's signing secret (from its env) - SESSION_COOKIE_PAYLOAD — JSON object: the session identity the app stores (email/name/user id — never a literal personal identity) - SESSION_COOKIE_SALT — the serializer's salt (omit when the app uses none) An app whose serializer differs (custom salt derivation, non-itsdangerous) needs its own public serializer — the pattern stays the same. """ import json import os import sys from itsdangerous import URLSafeTimedSerializer def main() -> None: secret = os.environ.get("SESSION_COOKIE_SECRET", "").strip() if not secret: sys.exit( "SESSION_COOKIE_SECRET is required and must not be blank — " "set it in the app's env and source it before running." ) try: payload = json.loads(os.environ.get("SESSION_COOKIE_PAYLOAD", "")) except json.JSONDecodeError as e: sys.exit(f"SESSION_COOKIE_PAYLOAD is not valid JSON ({e}) — set the session identity as a JSON object.") if not payload or not isinstance(payload, dict): sys.exit("SESSION_COOKIE_PAYLOAD must be a non-empty JSON object — the app stores it as the session identity.") salt = os.environ.get("SESSION_COOKIE_SALT") if salt is not None and not salt.strip(): sys.exit("SESSION_COOKIE_SALT is set but blank — set the app's salt or remove the variable.") serializer = URLSafeTimedSerializer(secret, salt=salt) if salt else URLSafeTimedSerializer(secret) sys.stdout.write(serializer.dumps(payload)) if __name__ == "__main__": main()

[github-actions]

PR Code Suggestions ✨

No code suggestions found for the PR.